Add support for sub-indexes #120

abhinav · 2023-10-15T04:28:40Z

Background

Sally renders two kinds of pages:

packages: These are for packages defined in sally.yaml
and any route under the package path.
indexes: These list available packages.

The latter--indexes was previously only supported at '/', the root page.

This leads to a slight UX issue:
if you have a package with a / in its name (e.g. net/metrics):

example.com/net/metrics gives you the package page
example.com/ lists net/metrics
However, example.com/net fails with a 404

This adds support for index pages on all parents of package pages.
Therefore, if example.com/net/metrics exists,
example.com/net will list all packages defined under that path.

Resolves #31

<details> <summary>Background</summary> Sally renders two kinds of pages: - packages: These are for packages defined in sally.yaml and any route under the package path. - indexes: These list available packages. The latter--indexes was previously only supported at '/', the root page. This leads to a slight UX issue: if you have a package with a / in its name (e.g. net/metrics): - example.com/net/metrics gives you the package page - example.com/ lists net/metrics - However, example.com/net fails with a 404 </details> This adds support for index pages on all parents of package pages. Therefore, if example.com/net/metrics exists, example.com/net will list all packages defined under that path. To make this possible, the internals have been rewritten to some degree: CreateHandler no longer builds an http.ServeMux with a bunch of pre-registered http.Handlers. It now builds a custom http.Handler with its own routing. Routing is backed by a new pathTree abstraction. pathTree is a specialized string-value storage with support for inheriting values from a parent. It has the following methods: - `Set(path, v)` associates v with path. - `Lookup(path)` returns the value at path, or the value inherited from the closest ancestor. - `ListByPath(path)` lists all values at or under the given path. For additional confidence in the implementation of pathTree, a property test based on rapid is included. Resolves uber-go#31

codecov · 2023-10-15T04:42:32Z

Codecov Report

Merging #120 (322469f) into master (f3a3b27) will increase coverage by 4.86%.
The diff coverage is 96.20%.

@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   74.03%   78.90%   +4.86%     
==========================================
  Files           3        3              
  Lines         104      128      +24     
==========================================
+ Hits           77      101      +24     
  Misses         23       23              
  Partials        4        4

Files	Coverage Δ
handler.go	`93.54% <96.20%> (+2.24%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Switch back to http.ServeMux for package handlers, and move the scanning logic to the index handler.

abhinav · 2023-10-17T04:22:09Z

The implementation has been simplified significantly.
I've eliminated the custom routing backend in favor of the same old ServeMux-backed router.
The performance difference from the custom router was non-existent and the cost in terms of complexity was a lot higher.

mway

LGTM

mway · 2023-10-18T15:49:53Z

handler.go

 	mux := http.NewServeMux()
-
-	pkgs := make([]packageInfo, 0, len(config.Packages))
+	pkgs := make([]*sallyPackage, 0, len(config.Packages))


nit: var group

I agree with var grouping when var form is used, but I think when we're initializing with non-zero values, var form is rarely used.

Per the style guide, we use var form for zero value initialization and := form for non-zero-value:
https://github.com/uber-go/guide/blob/master/style.md#use-var-for-zero-value-structs
https://github.com/uber-go/guide/blob/master/style.md#initializing-struct-references

(That links only talk about structs, but the guidance has been consistent on this.)

The readability in a var group suffers as soon as you add a type for any actual var-form-zero-values.

var ( mux = http.NewServeMux() pkgs = make(...) foo io.Reader )

It's fine as-is. :) Not trying to bikeshed.

My point was more that I don't personally see the value in segmenting the initializations here so I would probably group them all. I disagree about var group readability suffering when mixing initialization and zero-value declarations (and I think that ultimately leads to a lot more ad hoc initialization/declaration patterns).

mway · 2023-10-18T15:51:37Z

handler.go

+		modulePath := path.Join(baseURL, name)
+		docURL := "https://" + path.Join(config.Godoc.Host, modulePath)
+
+		pkg := &sallyPackage{
+			Name:       name,
+			Desc:       pkg.Desc,
+			ModulePath: modulePath,
+			DocURL:     docURL,
+			GitURL:     pkg.Repo,
+		}


nit: grouping

Suggested change

modulePath := path.Join(baseURL, name)

docURL := "https://" + path.Join(config.Godoc.Host, modulePath)

pkg := &sallyPackage{

Name: name,

Desc: pkg.Desc,

ModulePath: modulePath,

DocURL: docURL,

GitURL: pkg.Repo,

}

var (

modulePath = path.Join(baseURL, name)

docURL = "https://" + path.Join(config.Godoc.Host, modulePath)

pkg = &sallyPackage{

Name: name,

Desc: pkg.Desc,

ModulePath: modulePath,

DocURL: docURL,

GitURL: pkg.Repo,

}

)

Same comment as above. The var form is just less readable here. := for non-zero value initialization has always been the way.

handler.go

handler_test.go

Co-authored-by: Matt Way <[email protected]>

Co-authored-by: Sung Yoon Whang <[email protected]>

abhinav

Thanks for the reviews, @mway @sywhang.
I've applied some suggestions, and left comments on a couple.
Let me know what you think.

abhinav · 2023-10-18T17:50:54Z

handler.go

 	mux := http.NewServeMux()
-
-	pkgs := make([]packageInfo, 0, len(config.Packages))
+	pkgs := make([]*sallyPackage, 0, len(config.Packages))


I agree with var grouping when var form is used, but I think when we're initializing with non-zero values, var form is rarely used.

Per the style guide, we use var form for zero value initialization and := form for non-zero-value:
https://github.com/uber-go/guide/blob/master/style.md#use-var-for-zero-value-structs
https://github.com/uber-go/guide/blob/master/style.md#initializing-struct-references

(That links only talk about structs, but the guidance has been consistent on this.)

The readability in a var group suffers as soon as you add a type for any actual var-form-zero-values.

var ( mux = http.NewServeMux() pkgs = make(...) foo io.Reader )

abhinav · 2023-10-18T17:51:16Z

handler.go

+		modulePath := path.Join(baseURL, name)
+		docURL := "https://" + path.Join(config.Godoc.Host, modulePath)
+
+		pkg := &sallyPackage{
+			Name:       name,
+			Desc:       pkg.Desc,
+			ModulePath: modulePath,
+			DocURL:     docURL,
+			GitURL:     pkg.Repo,
+		}


Same comment as above. The var form is just less readable here. := for non-zero value initialization has always been the way.

abhinav · 2023-10-18T22:18:13Z

Per chat with @mway, will leave := initialization in place for cases where var form isn't used because a type isn't specified.

abhinav force-pushed the subindex branch from ae280ee to c8ca51f Compare October 15, 2023 04:40

Simplify the implementation

30ef30e

Switch back to http.ServeMux for package handlers, and move the scanning logic to the index handler.

abhinav force-pushed the subindex branch from d161aea to 30ef30e Compare October 17, 2023 04:20

abhinav requested a review from sywhang October 17, 2023 04:21

mway approved these changes Oct 18, 2023

View reviewed changes

sywhang approved these changes Oct 18, 2023

View reviewed changes

handler_test.go Outdated Show resolved Hide resolved

abhinav and others added 2 commits October 18, 2023 10:51

handler.go: Better formatting on packageTemplate

6c18e78

Co-authored-by: Matt Way <[email protected]>

handler_test: Sort+Equal => ElementsMatch

f6f83a4

Co-authored-by: Sung Yoon Whang <[email protected]>

abhinav commented Oct 18, 2023

View reviewed changes

oops

322469f

abhinav merged commit 5fbb57e into uber-go:master Oct 18, 2023
5 checks passed

abhinav deleted the subindex branch October 18, 2023 22:18

mway mentioned this pull request Oct 24, 2023

Release v1.5.0 #122

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sub-indexes #120

Add support for sub-indexes #120

abhinav commented Oct 15, 2023 •

edited

Loading

codecov bot commented Oct 15, 2023 •

edited

Loading

abhinav commented Oct 17, 2023

mway left a comment

mway Oct 18, 2023

abhinav Oct 18, 2023

mway Oct 18, 2023

mway Oct 18, 2023

abhinav Oct 18, 2023

abhinav left a comment

abhinav Oct 18, 2023

abhinav Oct 18, 2023

abhinav commented Oct 18, 2023

Add support for sub-indexes #120

Add support for sub-indexes #120

Conversation

abhinav commented Oct 15, 2023 • edited Loading

codecov bot commented Oct 15, 2023 • edited Loading

Codecov Report

abhinav commented Oct 17, 2023

mway left a comment

Choose a reason for hiding this comment

mway Oct 18, 2023

Choose a reason for hiding this comment

abhinav Oct 18, 2023

Choose a reason for hiding this comment

mway Oct 18, 2023

Choose a reason for hiding this comment

mway Oct 18, 2023

Choose a reason for hiding this comment

abhinav Oct 18, 2023

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

abhinav Oct 18, 2023

Choose a reason for hiding this comment

abhinav Oct 18, 2023

Choose a reason for hiding this comment

abhinav commented Oct 18, 2023

abhinav commented Oct 15, 2023 •

edited

Loading

codecov bot commented Oct 15, 2023 •

edited

Loading