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.

Sign up for GitHub

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

Jump to bottom

golang: add support for passthrough args for gofmt #18409

Merged
merged 3 commits into from Mar 6, 2023
Merged

golang: add support for passthrough args for gofmt #18409

merged 3 commits into from Mar 6, 2023

Conversation

AlexTereshenkov
Copy link
Member

@AlexTereshenkov AlexTereshenkov commented Mar 4, 2023 •
edited

Closes #12822

This PR makes it possible to pass (some) arguments to the gofmt program to be able to use the flags that gofmt provides.

E.g. running gofmt -s on

fmt.Println(version[1:len(version)])

would result in simplified (-s) code:

fmt.Println(version[1:])

The interface:

$ ./pants fmt --gofmt-args="-s" cmd/greeter_en/main.go

or with the pants.toml:

[gofmt]
args = ["-s", "-e"]

@AlexTereshenkov AlexTereshenkov marked this pull request as ready for review March 4, 2023 17:03
@tdyas tdyas self-requested a review March 5, 2023 20:00
danxmoran
danxmoran reviewed Mar 6, 2023
View reviewed changes
src/python/pants/backend/go/lint/gofmt/rules.py Outdated
pass


SupportedGoFmtArgs = FrozenOrderedSet(("-e", "-r", "-s"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only support some args? Can you add a comment explaining?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some options like -w are not style related and are used by the rule code to change how gofmt operates. This type of option should not be passed through or else it may interfere with the rule.

src/python/pants/backend/go/lint/gofmt/subsystem.py Outdated
@@ -11,3 +11,4 @@ class GofmtSubsystem(Subsystem):
help = "Gofmt-specific options."

skip = SkipOption("fmt", "lint")
args = ArgsListOption(example="-s")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we incorporate the list of supported args in this help-string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent idea, thank you! This is now done :)

tdyas
tdyas reviewed Mar 6, 2023
View reviewed changes
Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

Looks generally fine.

Side note: Not a big deal but since I wrote this code and am still a maintainer, please do add me as a reviewer next time.

src/python/pants/backend/go/lint/gofmt/rules.py Outdated
pass


SupportedGoFmtArgs = FrozenOrderedSet(("-e", "-r", "-s"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Style: Identifiers for constants should be in all capitals.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

src/python/pants/backend/go/lint/gofmt/rules.py Outdated
pass


SupportedGoFmtArgs = FrozenOrderedSet(("-e", "-r", "-s"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Some options like -w are not style related and are used by the rule code to change how gofmt operates. This type of option should not be passed through or else it may interfere with the rule.

@AlexTereshenkov
Copy link
Member Author

Looks generally fine.

Side note: Not a big deal but since I wrote this code and am still a maintainer, please do add me as a reviewer next time.

Thank you for taking a look! I was sure I've tagged you along. Very sorry that wasn't the case and of course I intend to ask you for Go related reviews :)

tdyas
tdyas approved these changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@tdyas tdyas left a comment

Choose a reason for hiding this comment

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

Approving with two minor comments.

src/python/pants/backend/go/lint/gofmt/subsystem.py
from pants.option.subsystem import Subsystem
from pants.util.ordered_set import FrozenOrderedSet

SUPPORTED_GOFMT_ARGS = FrozenOrderedSet(("-e", "-r", "-s"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Python's frozenset should be sufficient here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I should have mentioned that. The reason I've used an ordered one is to be able to confirm that this thing appears in the exception message when running a test with invalid arguments passed.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I also wanted the help message to be consistent and output the arguments in the same order all the time.

src/python/pants/backend/go/lint/gofmt/subsystem.py Outdated
@@ -11,3 +14,7 @@ class GofmtSubsystem(Subsystem):
help = "Gofmt-specific options."

skip = SkipOption("fmt", "lint")
args = ArgsListOption(
example="-s -e",
extra_help=f"Only the following style related options are supported: {tuple(SUPPORTED_GOFMT_ARGS)}.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the following would look nicer than the tuple?

", ".join([f"`{x}`" for x in xs])

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this looks great to me!

  --gofmt-args="[<shell_str>, <shell_str>, ...]"
  PANTS_GOFMT_ARGS
  args
      default: []
      current value: []
      Arguments to pass directly to gofmt, e.g. `--gofmt-args='-s -e'`.
      
      Only the following style related options are supported: `-e`, `-r`, `-s`.

@AlexTereshenkov AlexTereshenkov merged commit b92a0a0 into pantsbuild:main Mar 6, 2023
@AlexTereshenkov AlexTereshenkov deleted the backend/golang/add-args-to-gofmt branch March 6, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdyas tdyas tdyas approved these changes

@danxmoran danxmoran danxmoran approved these changes

@thejcannon thejcannon Awaiting requested review from thejcannon

Assignees
No one assigned
Labels
backend: Go Go backend-related issues category:new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Go: support passthrough args for gofmt
3 participants
@AlexTereshenkov @tdyas @danxmoran