Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(internal/godocfx): add support for other modules #4290

Merged
merged 6 commits into from Jun 22, 2021
Merged

Conversation

@tbpg
Copy link
Collaborator

@tbpg tbpg commented Jun 21, 2021

I chose to process all modules at the same time, rather than in separate
invocations of godocfx, so we wouldn't have to make duplicate requests
to the module index.

The docs.metadata generation will probably need to be updated in a
future PR to support different final URL destinations.

I chose to process all modules at the same time, rather than in separate
invocations of godocfx, so we wouldn't have to make duplicate requests
to the module index.

The docs.metadata generation will probably need to be updated in a
future PR to support different final URL destinations.
@tbpg tbpg requested a review from codyoss Jun 21, 2021
@tbpg tbpg requested a review from as a code owner Jun 21, 2021
@google-cla google-cla bot added the cla: yes label Jun 21, 2021
@tbpg tbpg requested a review from shollyman Jun 21, 2021
Copy link
Contributor

@shollyman shollyman left a comment

My comments are largely borne of lack of familiarity with the project, feel free to ignore if they're irrelevant.

Loading

modPath = strings.TrimSuffix(modPath, "/...") // No /... needed.
mods = []indexEntry{
{
modPath = strings.TrimSuffix(modPath, "/...") // No /... needed.
Copy link
Contributor

@shollyman shollyman Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclear to me end to end how module strings are moving through the system, do you ever need to sanitize trailing comments?

Loading

Copy link
Collaborator Author

@tbpg tbpg Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by trailing comments. This comes from a command line argument. This makes sure that if the user did include the /... at the end, we handle it transparently. A previous version of this command passed the argument through directly, and I made use of /..., so I didn't want to break that.

Loading

@@ -49,7 +49,7 @@ func (f *fakeTS) put(context.Context, time.Time) error {
func TestNewModules(t *testing.T) {
ic := fakeIC{}
ts := &fakeTS{}
entries, err := newModules(context.Background(), ic, ts, "cloud.google.com")
entries, err := newModules(context.Background(), ic, ts, []string{"cloud.google.com"})
Copy link
Contributor

@shollyman shollyman Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe include an entry with explicit versioning and maybe something that exercises the filter paths?

Loading

Copy link
Collaborator Author

@tbpg tbpg Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test really just makes sure the paging logic works for the index. It doesn't exercise the prefix logic, which is a gap in coverage. Can try to figure out how to add a test for it (need to refactor the HTTP calls) if you feel strongly.

Loading

@@ -28,7 +28,7 @@ import (

// indexer gets a limited list of entries from index.golang.org.
type indexer interface {
get(prefix string, since time.Time) (entries []indexEntry, last time.Time, err error)
get(prefixes []string, since time.Time) (entries []indexEntry, last time.Time, err error)
Copy link
Contributor

@shollyman shollyman Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are overlapping prefixes an issue that may need consideration?

Loading

Copy link
Collaborator Author

@tbpg tbpg Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. We won't process duplicate modules because we don't process each prefix independently -- if a module matches any prefix, it's included.

Loading

Copy link
Collaborator Author

@tbpg tbpg left a comment

Thank you!

Loading

@@ -28,7 +28,7 @@ import (

// indexer gets a limited list of entries from index.golang.org.
type indexer interface {
get(prefix string, since time.Time) (entries []indexEntry, last time.Time, err error)
get(prefixes []string, since time.Time) (entries []indexEntry, last time.Time, err error)
Copy link
Collaborator Author

@tbpg tbpg Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think so. We won't process duplicate modules because we don't process each prefix independently -- if a module matches any prefix, it's included.

Loading

@@ -49,7 +49,7 @@ func (f *fakeTS) put(context.Context, time.Time) error {
func TestNewModules(t *testing.T) {
ic := fakeIC{}
ts := &fakeTS{}
entries, err := newModules(context.Background(), ic, ts, "cloud.google.com")
entries, err := newModules(context.Background(), ic, ts, []string{"cloud.google.com"})
Copy link
Collaborator Author

@tbpg tbpg Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test really just makes sure the paging logic works for the index. It doesn't exercise the prefix logic, which is a gap in coverage. Can try to figure out how to add a test for it (need to refactor the HTTP calls) if you feel strongly.

Loading

modPath = strings.TrimSuffix(modPath, "/...") // No /... needed.
mods = []indexEntry{
{
modPath = strings.TrimSuffix(modPath, "/...") // No /... needed.
Copy link
Collaborator Author

@tbpg tbpg Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by trailing comments. This comes from a command line argument. This makes sure that if the user did include the /... at the end, we handle it transparently. A previous version of this command passed the argument through directly, and I made use of /..., so I didn't want to break that.

Loading

Copy link
Member

@codyoss codyoss left a comment

LGTM, thanks!

Loading

@tbpg tbpg added the automerge label Jun 22, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit d52bae6 into master Jun 22, 2021
5 checks passed
Loading
@gcf-merge-on-green gcf-merge-on-green bot deleted the godocfx branch Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants