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

Enforce alphabetically ordered cargo deps #71

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

dknopik
Copy link

@dknopik dknopik commented Dec 12, 2024

Shamelessly stolen from @macladson's PR here: sigp/lighthouse#6678

@@ -192,3 +192,18 @@ jobs:
# cache-target: release
# - name: Run Makefile to trigger the bash script
# run: make cli
cargo-sort:

Choose a reason for hiding this comment

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

What do you think about moving this job and the check-code to a new workflow as they are more about code quality than testing?

Copy link
Author

Choose a reason for hiding this comment

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

Don't really have an opinion on that. @magick93, what do you think?

Choose a reason for hiding this comment

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

IMO splitting it wont add too much value, though I'm happy to do it.

The best value - in the case of test and code qual. comes from giving rapid feedback to the dev. It its not too computationally intensive maybe a pre-commit hook may be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

good comment diego, let's merge for now but as workflows become more refined we can revisit

Copy link
Contributor

@jking-aus jking-aus left a comment

Choose a reason for hiding this comment

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

lgtm

@jking-aus jking-aus merged commit 6ec4b6a into sigp:unstable Dec 16, 2024
9 checks passed
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.

4 participants