Edit Type/Function Documentation in the Config Package #56
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Aneurysm9
reviewed
May 24, 2021
There was a problem hiding this comment.
I think the EOL comments were probably fine as they were, but no real harm in rearranging them. I'm not sure internal comments need proper punctuation and sentence structure, either. I'm fine with them, but you may get some pushback upstream depending on how used to using this style the maintainers are.
config/configauth/configauth.go
Outdated
| // GetServerAuthenticator attempts to select the appropriate from the list of extensions, based on the requested extension name. | ||
| // If an authenticator is not found, an error is returned. | ||
| // GetServerAuthenticator attempts to select the appropriate ServerAuthenticator from the list of extensions, based on the requested extension name. | ||
| // If authenticator is not found, an error is returned. |
There was a problem hiding this comment.
Suggested change
| // If authenticator is not found, an error is returned. | |
| // If an authenticator is not found, an error is returned. |
I think this is correct. Alternately:
Suggested change
| // If authenticator is not found, an error is returned. | |
| // If no authenticator is found, an error is returned. |
config/configauth/mock_clientauth.go
Outdated
| func (m *MockClientAuthenticator) Shutdown(ctx context.Context) error { | ||
| return nil | ||
| } | ||
|
|
||
| // RoundTripper for the MockClientAuthenticator either returns error if the mock authenticator is forced to or | ||
| // returns the supplied resultRoundTripper. | ||
| // RoundTripper for the MockClientAuthenticator either returns an error if the mock authenticator is forced to |
There was a problem hiding this comment.
Suggested change
| // RoundTripper for the MockClientAuthenticator either returns an error if the mock authenticator is forced to | |
| // RoundTripper for the MockClientAuthenticator either returns an error if the mock authenticator is configured to |
config/configauth/mock_clientauth.go
Outdated
| func (m *MockClientAuthenticator) RoundTripper(base http.RoundTripper) (http.RoundTripper, error) { | ||
| if m.MustError { | ||
| return nil, errMockError | ||
| } | ||
| return m.ResultRoundTripper, nil | ||
| } | ||
|
|
||
| // PerRPCCredentials for the MockClientAuthenticator either returns error if the mock authenticator is forced to or | ||
| // returns the supplied resultPerRPCCredentials. | ||
| // PerRPCCredentials for the MockClientAuthenticator either returns an error if the mock authenticator is forced to |
There was a problem hiding this comment.
Suggested change
| // PerRPCCredentials for the MockClientAuthenticator either returns an error if the mock authenticator is forced to | |
| // PerRPCCredentials for the MockClientAuthenticator either returns an error if the mock authenticator is configured to |
config/configgrpc/gzip.go
Outdated
| @@ -15,6 +15,6 @@ | |||
| package configgrpc | |||
|
|
|||
| import ( | |||
| // import the gzip package with auto-registers the gzip grpc compressor | |||
| // Import the gzip package with auto-registers the gzip gRPC compressor. | |||
There was a problem hiding this comment.
Suggested change
| // Import the gzip package with auto-registers the gzip gRPC compressor. | |
| // Import the gzip package which auto-registers the gzip gRPC compressor. |
There was a problem hiding this comment.
Thanks for catching this!! Addressed the other comments as well :D
config/exporter.go
Outdated
Comment on lines
28
to
29
| // Specific exporters can embed this struct, and extend it with more fields if needed. | ||
| // When embedded in the exporter config, it must be with `mapstructure:",squash"` tag. |
There was a problem hiding this comment.
Suggested change
| // Specific exporters can embed this struct, and extend it with more fields if needed. | |
| // When embedded in the exporter config, it must be with `mapstructure:",squash"` tag. | |
| // Specific exporters can embed this struct and extend it with more fields if needed. | |
| // When embedded in the exporter config it must be with `mapstructure:",squash"` tag. |
I don't think the commas are necessary here.
There was a problem hiding this comment.
I agree that the first sentence does not need the comma (parallel verbs), but I believe it wouldn't hurt readability to have the second comma (Dependent clause, Independent clause)!
Suggested change
| // Specific exporters can embed this struct, and extend it with more fields if needed. | |
| // When embedded in the exporter config, it must be with `mapstructure:",squash"` tag. | |
| // Specific exporters can embed this struct and extend it with more fields if needed. | |
| // When embedded in the exporter config, it must be with `mapstructure:",squash"` tag. |
| @@ -28,7 +28,7 @@ type Extensions map[ComponentID]Extension | |||
|
|
|||
| // ExtensionSettings defines common settings for an extension configuration. | |||
| // Specific processors can embed this struct and extend it with more fields if needed. | |||
| // When embedded in the extension config it must be with `mapstructure:",squash"` tag. | |||
| // When embedded in the extension config, it must be with `mapstructure:",squash"` tag. | |||
There was a problem hiding this comment.
Suggested change
| // When embedded in the extension config, it must be with `mapstructure:",squash"` tag. | |
| // When embedded in the extension config it must be with `mapstructure:",squash"` tag. |
There was a problem hiding this comment.
Same comment as above; I think it would be grammatically correct to have a comma, which may improve readability slightly!
euniceek
force-pushed
the
config-package-typos
branch
from
97a4625
to
dc13524
Compare
alolita
approved these changes
May 25, 2021
euniceek
force-pushed
the
config-package-typos
branch
from
09723e8
to
03ad2cb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR is first of the three PRs that will be filed to review and improve the Config package. Changes were made to update the documentation of type and func declarations in the configuration package and all of its sub-packages, with a focus on ensuring that the documentation follows the best practices recommended by Go, and that there is no grammar mistakes.
Link to tracking Issue:
Addresses part of # 3051