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

add return parameter names to Gen functions #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zephyrtronium
Copy link

34fab66 added a second string return to Gen functions. Use named return parameters to document the meanings of each.

Notably, GenSqlserver is the only function that actually uses the new return. Name the second string _ everywhere else to clarify that it can and should be ignored.

Fixes #30.

34fab66 added a second string return to Gen functions. Use named return
parameters to document the meanings of each.

Notably, GenSqlserver is the only function that actually uses the new
return. Name the second string _ everywhere else to clarify that it can
and should be ignored.

Fixes xo#30.
@@ -21,7 +21,7 @@ var Stat = func(name string) (fs.FileInfo, error) {

// GenScheme returns a func that generates a scheme:// style DSN from the
// passed URL.
func GenScheme(scheme string) func(*URL) (string, string, error) {
func GenScheme(scheme string) func(*URL) (dsn, _ string, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Named return parameters are usually used with a naked return statement. Mixing both might lead to confusing behavior, where we'd return a different value than what's held in the return variable. I'm not sure if this is a good change, if it's only meant to improve readability.

Copy link
Author

Choose a reason for hiding this comment

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

I disagree that named parameters are usually used with naked returns. Effective Go describes them as "documentation" and includes an example of exactly this kind of scenario, where multiple returns share a type but the names disambiguate their respective meanings.

The advice I've seen (and given, as a community volunteer) has been roughly, "Named parameters serve as excellent documentation. Naked returns are ok for very short functions and generated code but should be avoided otherwise." Code I've encountered has generally followed the same rule. This is also the advice in the Go team's own style guide, as well as Google's.

I think the next best solution to #30 is adding doc comments to each Gen function describing the meanings of the parameters. That takes many more words and can drift from truth in the event of future API changes. The approach I took also has the advantage that editors will often show return parameter names even in places where they don't show doc comments, like in autocomplete and snippets. In other words, this isn't only about readability but also usability as well.

Another option would be to describe it in the package documentation or readme. Currently, neither mentions the Gen functions at all, since it's a lower level interface. It's also far from where the functions would be used; you'd have to go to pkg.go.dev to check the meanings.

If you'd still prefer a different approach, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for all the references, that's convincing!

@kenshaw
Copy link
Member

kenshaw commented Sep 5, 2023

@zephyrtronium I'm surprised to find out that anyone is actually using this API, but that's cool! I apologize for changing the dburl API, but it was necessary as I had to deal/contend with Microsoft's SQL Server driver that shoe horns 2 drivers into one package. I did not mean to complicate development for you.

With regards to your PR, it would be inconsistent to name and document only some parameters. I'll add some documentation now.

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.

no documentation on v0.16.0 Gen return parameters
3 participants