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

Run pytest before ruff linter checks #267

Merged
merged 1 commit into from
Feb 27, 2025

Conversation

jordandsullivan
Copy link
Collaborator

I want to test the functionality of a change before checking the formatting. In this way you get a failure like on an unused import before the code itself runs: https://github.com/unitaryfund/ucc/actions/runs/13554895918/job/37886926715

I want to test the functionality of a change before checking the formatting. In this way you get a failure like on an unused import before the code itself runs:
https://github.com/unitaryfund/ucc/actions/runs/13554895918/job/37886926715
@jordandsullivan jordandsullivan marked this pull request as ready for review February 26, 2025 23:01
Copy link
Collaborator

@bachase bachase left a comment

Choose a reason for hiding this comment

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

LGTM.

In addition, if you hadn't already, you could consider:

  1. Depending on your IDE, you may be able to set the ruff format and ruff linter to run automatically whenever you save a file.
  2. You can enable pre-commit hooks (mentioned in our docs here) that will automatically format and fix any linter issues right before any calls to git commit complete (or will prevent commit if it would cause a conflict). The benefit there is it catches it earlier, and avoids an extra commit for fixing.

cc @natestemen and @Misty-W if they have any different workflows. We can consider documenting this more in the docs or README as well.

@natestemen
Copy link
Member

natestemen commented Feb 27, 2025

Okay by me. Another way to achieve this is to use continue-on-error, so both the tests, and the linting can run. It does look like there is some uncertainty on the behavior of this flag from stackoverflow, but we could test it!

@jordandsullivan
Copy link
Collaborator Author

I would rather just stick with the order for now rather than test a new flag. I will update my pre-commit hooks though thanks!

@jordandsullivan jordandsullivan merged commit 6c56e70 into main Feb 27, 2025
2 checks passed
@jordandsullivan jordandsullivan deleted the js-run-tests-before-linter branch February 27, 2025 18:34
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.

3 participants