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

cli: fmt -check should return early on diff #16174

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Conversation

tgross
Copy link
Member

@tgross tgross commented Feb 14, 2023

Fixes #16170

The nomad fmt -check command incorrectly writes to file because we didn't
return before writing the file on a diff. Fix this bug and update the command
internals to differentiate between the write-to-file and write-to-stdout code
paths, which are activated by different combinations of options and flags.

The docstring for the -list and -write flags is also unclear and can be
easily misread to be the opposite of the actual behavior. Clarify this and fix
up the docs to match.

This changeset also refactors the tests quite a bit so as to make the test
outputs clear when something is incorrect and avoid reusing the UI state.

The `nomad fmt -check` command incorrectly writes to file because we didn't
return before writing the file on a diff. Fix this bug and update the command
internals to differentiate between the write-to-file and write-to-stdout code
paths, which are activated by different combinations of options and flags.

The docstring for the `-list` and `-write` flags is also unclear and can be
easily misread to be the opposite of the actual behavior. Clarify this and fix
up the docs to match.

This changeset also refactors the tests quite a bit so as to make the test
outputs clear when something is incorrect.
Copy link
Contributor

@angrycub angrycub left a comment

Choose a reason for hiding this comment

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

Needs a couple of small touches to the docs markup, and I shared some testing thoughts that are totally optional.

command/fmt_test.go Show resolved Hide resolved
command/fmt_test.go Outdated Show resolved Hide resolved
website/content/docs/commands/fmt.mdx Show resolved Hide resolved
website/content/docs/commands/fmt.mdx Show resolved Hide resolved
Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.4.x backport to 1.4.x release line theme/cli type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nomad fmt should return non zero exit code when changing files
3 participants