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

Conversation

tbpg
Copy link
Contributor

@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 June 21, 2021 17:53
@tbpg tbpg requested a review from a team as a code owner June 21, 2021 17:53
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 21, 2021
@tbpg tbpg requested a review from shollyman June 21, 2021 18:15
Copy link
Contributor

@shollyman shollyman left a comment

Choose a reason for hiding this comment

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

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

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

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?

Copy link
Contributor Author

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.

@@ -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

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?

Copy link
Contributor Author

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.

@@ -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

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@tbpg tbpg left a comment

Choose a reason for hiding this comment

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

Thank you!

@@ -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 Author

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.

@@ -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 Author

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.

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

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.

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tbpg tbpg added the automerge Merge the pull request once unit tests and other checks pass. label Jun 22, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit d52bae6 into master Jun 22, 2021
@gcf-merge-on-green gcf-merge-on-green bot deleted the godocfx branch June 22, 2021 14:34
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants