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

fmt: format load strings in tests #282

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichaHoffmann
Copy link
Contributor

Adds a small tool so we can auto-format indentation for load statements.

@MichaHoffmann MichaHoffmann force-pushed the mhoffm-autoformat-load-directives branch 6 times, most recently from c1f52bb to ba092b5 Compare May 31, 2023 18:29
Signed-off-by: Michael Hoffmann <mhoffm@posteo.de>
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-autoformat-load-directives branch from ba092b5 to daeac04 Compare May 31, 2023 18:34
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Nice!

We should probably hook this into our CI as well, by adding it to format target.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can just put this in scripts dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can do!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe making this into its own module, so that it is separately installable might be better, in case we want to upstream it for use in prometheus/prometheus at some point? 🙂

return true
}
cont := s.Value[1 : len(s.Value)-1]
if !strings.HasPrefix(cont, "load") {
Copy link
Member

Choose a reason for hiding this comment

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

I think there are other directives as well like clear and eval: https://github.com/prometheus/prometheus/blob/main/promql/test.go#L252

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was intending it only to format our tests for now; to do it properly we would need to be able to parse those load directives.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Maybe we can to run this during CI and error out if there are any changes after running this tool?

@fpetkovski
Copy link
Collaborator

Running it in the CI seems like a good idea 👍

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

4 participants