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

Clean up godoc #82

Merged
merged 2 commits into from
Sep 10, 2021
Merged

Clean up godoc #82

merged 2 commits into from
Sep 10, 2021

Conversation

benjaminjkraft
Copy link
Collaborator

Summary:

Before open-sourcing, we want to make sure that (a) GoDoc looks
reasonable, and (b) everything in the API is something we want to commit
to. In this commit, I do some miscellaneous cleanup on both fronts;
this does involve a few breaking changes to the programmatic API (better
now than once it has users). In future commits, I'll likely move the
documentation for genqlient.yaml and @genqlient to clearer places,
and make GenqlientDirective private, such that GoDoc is really only
for programmatic users.

Fixes #25.

Issue: #25

Test plan:

make check

@benjaminjkraft benjaminjkraft self-assigned this Sep 9, 2021
@benjaminjkraft benjaminjkraft linked an issue Sep 9, 2021 that may be closed by this pull request
@mahtabsabet mahtabsabet self-requested a review September 9, 2021 17:16
@benjaminjkraft benjaminjkraft requested a review from jvoll September 9, 2021 17:35
@@ -204,7 +204,7 @@ func TestGenerateWithConfig(t *testing.T) {
config := test.config
t.Run(test.name, func(t *testing.T) {
err := config.ValidateAndFillDefaults(
filepath.Join(dataDir, test.fakeConfigFilename))
filepath.Join(dataDir, config.baseDir))
Copy link
Contributor

Choose a reason for hiding this comment

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

config.baseDir is empty at this point? (Also, this value is used to set baseDir, so it seems odd to use baseDir in this context.) I think this can just be dataDir.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah this is kind of sneaky: I'm setting it in some cases above. It's probably better to just have a field in the test-case struct for that; I'll make that change.

Copy link
Member

@jvoll jvoll left a comment

Choose a reason for hiding this comment

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

All seems reasonable to me.

Base automatically changed from benkraft.init to main September 10, 2021 22:49
Before open-sourcing, we want to make sure that (a) GoDoc looks
reasonable, and (b) everything in the API is something we want to commit
to.  In this commit, I do some miscellaneous cleanup on both fronts;
this does involve a few breaking changes to the programmatic API (better
now than once it has users).  In future commits, I'll likely move the
documentation for `genqlient.yaml` and `@genqlient` to clearer places,
and make `GenqlientDirective` private, such that GoDoc is really only
for programmatic users.

Fixes #25.

Issue: #25

Test plan: make check

Reviewers: marksandstrom, miguel, adam
@benjaminjkraft benjaminjkraft merged commit d41cf63 into main Sep 10, 2021
@benjaminjkraft benjaminjkraft deleted the benkraft.docs-1 branch September 10, 2021 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up API
3 participants