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

Clothe returns #287

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Clothe returns #287

wants to merge 11 commits into from

Conversation

flimzy
Copy link

@flimzy flimzy commented Oct 20, 2023

Fixes #285


At present, this transformation ignores naked returns in functions with blank-named return values (i.e. func() (_ int, err error)) because rewriting them requires a literal zero value, which seemed unnecessary to evaluate the usefulness of this rule. It can be updated if we decide to move forward with this.

There's probably also room to improve the algorithm used to determine the nearest function parent to a given return statement. I didn't see any prior art within this project to copy, so I chose an easy option to implement, which I expect is enough to evaluate the usefulness. And we can optimize if deemed necessary.

@flimzy
Copy link
Author

flimzy commented Oct 20, 2023

This is failing to properly adjust the location of comments after inserting the return values. I'll continue working on that.

@flimzy
Copy link
Author

flimzy commented Oct 23, 2023

The comments issue should be resolved with a727581 (Thanks @mvdan for the help on Slack!)

@flimzy
Copy link
Author

flimzy commented Oct 23, 2023

See flimzy@64e6495 for what this new transform does on the gofumpt repo itself. I'll do further testing on a larger collection of repos when time permits.

@flimzy
Copy link
Author

flimzy commented Oct 24, 2023

I've run this change against the stdlib, as well as the repos in this list. I found two panics, which already existed in gofumpt prior to this change. Otherwise, the output appears as expected and, in my subjective opinion, much easier to read.

I could share some diffs, but they're bulky and boring to read, honestly.

If you'd like me to do any additional investigation, please let me know.

Copy link
Owner

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

SGTM, just some thoughts, in particular I'm worried about PathEnclosingInterval :)

format/format.go Outdated Show resolved Hide resolved
format/format.go Outdated Show resolved Hide resolved
format/format.go Outdated Show resolved Hide resolved
@flimzy
Copy link
Author

flimzy commented Apr 16, 2024

Anything else before merging this one, @mvdan ?

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.

Proposal: Clothe naked returns
2 participants