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

feat: add flag to print version #117

Merged
merged 5 commits into from May 29, 2023
Merged

feat: add flag to print version #117

merged 5 commits into from May 29, 2023

Conversation

kachick
Copy link
Contributor

@kachick kachick commented May 29, 2023

When I addressed to create asdf plugin, the way to ensure the version of the binary is not found.
This feature always helps me. So I added a simple flag to print the version.

https://github.com/kachick/asdf-yamlfmt
asdf-vm/asdf-plugins#829

| Dry Run | `-dry` | `yamlfmt -dry .` | Use [Dry Run](#dry-run) mode |
| Lint | `-lint` | `yamlfmt -lint .` | Use [Lint](#lint) mode |
| Quiet Mode | `-quiet` | `yamlfmt -dry -quiet .` | Use quiet mode. Only has effect in Dry Run or Lint modes. |
| Read Stdin | `-in` | `cat x.yaml \| yamlfmt -in` | Read input from stdin and output result to stdout. |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line includes fix of #119

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, didn't notice this

Copy link
Collaborator

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I have needed this for a while but haven't gotten to it. Eventually I will change this to use ldflags in the build but until then this is a good stopgap.

I think instead of printing the version as an operation, it would be better if the version flag was simply detected immediately after the flags are parsed in main.go, at which point it will print out the version. It saves a lot of extra work that the program doesn't need to do if all it's doing is printing out the version. Would you be able to make that change?

| Dry Run | `-dry` | `yamlfmt -dry .` | Use [Dry Run](#dry-run) mode |
| Lint | `-lint` | `yamlfmt -lint .` | Use [Lint](#lint) mode |
| Quiet Mode | `-quiet` | `yamlfmt -dry -quiet .` | Use quiet mode. Only has effect in Dry Run or Lint modes. |
| Read Stdin | `-in` | `cat x.yaml \| yamlfmt -in` | Read input from stdin and output result to stdout. |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, didn't notice this

@kachick
Copy link
Contributor Author

kachick commented May 29, 2023

@braydonk Thanks for your reviewing!

I agree that your suggestion is good 👍 . So I applied patch as c5876f3
Since this change, given version flag ignores other flags except help and immediately exit with the version displayed. Is this correct understanding?

Copy link
Collaborator

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Since this change, given version flag ignores other flags except help and immediately exit with the version displayed. Is this correct understanding?

Yes that's right! Just a couple small comments there then I think this will be good to go.

return err
}

os.Exit(0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can just return nil here; it exits the program the same way.

Suggested change
os.Exit(0)
return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I'm learning golang again from this project... 🙏

f616842

Copy link
Collaborator

Choose a reason for hiding this comment

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

I appreciate you making the effort to contribute in something that isn't your main language! Means a lot 😄

@@ -34,6 +38,15 @@ func run() error {
configureHelp()
flag.Parse()

if *flagVersion {
_, err := fmt.Printf("%s\n", version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

By convention in this repo I'm not too worried about errors from fmt.Printf, so just doing the print here is fine.

I think this can also be simplified to fmt.Println(version) if I'm not mistaken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes simple code 👍

f616842

Copy link
Collaborator

@braydonk braydonk left a comment

Choose a reason for hiding this comment

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

Thank you for the PR and for working with me in the review!

@braydonk braydonk merged commit 8cc9885 into google:main May 29, 2023
4 checks passed
@kachick
Copy link
Contributor Author

kachick commented May 29, 2023

Thank you for your polite review! 🙏

@kachick kachick deleted the add-version-flag branch May 29, 2023 14:36
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.

None yet

2 participants