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 Formatting Fail Flag for CI Use #923

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Conversation

benyanke
Copy link
Contributor

@benyanke benyanke commented Sep 16, 2024

A common use with linters and formatters is checking in CI and failing if the formatter required any changed. This ensures that branches are never merged without the formatter being run.

This PR implements this functionality by tracking the number of files changed, and allowing the user to return an error exit code if -fail is provided, subsequently failing the CI job if formatting was not done before commit.

This PR also has the side benefit of providing additional user situational awareness as it now prints the number of changed files on every format run, even if the CI-fail flag is not used.

After merge, users can add the following to their CI to validate template formatting.

templ fmt -fail .

Results

fmt's help now looks like this:

./templ fmt --help
usage: templ fmt [<args> ...]

Format all files in directory:

  templ fmt .

Format stdin to stdout:

  templ fmt < header.templ

Format file or directory to stdout:

  templ fmt -stdout FILE

Args:
  -stdout
    Prints to stdout instead of in-place format
  -stdin-filepath
    Provides the formatter with filepath context when using -stdout.
    Required for organising imports.
  -v
    Set log verbosity level to "debug". (default "info")
  -log-level
    Set log verbosity level. (default "info", options: "debug", "info", "warn", "error")
  -w
    Number of workers to use when formatting code. (default runtime.NumCPUs).
  -fail
    Fails with an error exit code if files are changed for use in CI.
  -help
    Print help and exit.

A normal run now looks like this (note the changed field) if no changes made.

./templ fmt .
(✓) Format Complete [ count=4 errors=0 changed=0 duration=11.045765ms ]

A normal run now looks like this (note the changed field) if changes are made.

./templ fmt .
(✓) Format Complete [ count=4 errors=0 changed=1 duration=11.045765ms ]

The new flag run now looks like this, if changes are made.

./templ fmt -fail .
(✗) Templates were valid but not properly formatted [ count=4 changed=1 errors=0 duration=19.007765ms ]

In the above case, the unix exit code is also 1 instead of the normal 0.

@joerdav
Copy link
Collaborator

joerdav commented Sep 17, 2024

Thanks for the contribution! A couple of points from me:

@benyanke
Copy link
Contributor Author

I'm open to another flag, sure. I'll leave that up to you, just let me know. I'd suggest -ci or -fail.

I can look at adding tests too.

@joerdav
Copy link
Collaborator

joerdav commented Sep 17, 2024

I like -fail to let's go with that. Thanks!

@benyanke
Copy link
Contributor Author

pushed the flag change now. Working on tests later.

@benyanke
Copy link
Contributor Author

Ok, I've added both a pass and fail test case @joerdav , as well as adding the flag to the args structs of the other related test for completeness. Let me know what you think.

cmd/templ/main.go Outdated Show resolved Hide resolved
@joerdav
Copy link
Collaborator

joerdav commented Sep 18, 2024

I think this is my last comment now, thanks for amending based on the last ones! I think the current wording implies it is "for CI", but I think that's just one use case.

I think it may be better to use wording like "This flag does X, this could be used in CI to check blah blah blah" rather than "This flag does X for use in CI"

Does that make sense?

@benyanke
Copy link
Contributor Author

Yes, that makes sense, and better conveys the meaning I was hoping to convey. I like that wording. good feedback!

Co-authored-by: Joe Davidson <[email protected]>
@benyanke
Copy link
Contributor Author

Ok, ready to rock and roll.

@joerdav joerdav merged commit ff29e12 into a-h:main Sep 18, 2024
4 checks passed
@benyanke benyanke deleted the add-ci-fail-flag branch September 18, 2024 20:38
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.

2 participants