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

Issues with linting CI #80

Open
TomNicholas opened this issue Sep 14, 2024 · 7 comments
Open

Issues with linting CI #80

TomNicholas opened this issue Sep 14, 2024 · 7 comments
Assignees
Labels
CI Continuous Integration or tooling

Comments

@TomNicholas
Copy link
Contributor

  1. I'm not convinced that the formatting linter is running correctly,
  2. There is no separate test in the CI for pre-commit
@TomNicholas TomNicholas added the CI Continuous Integration or tooling label Sep 14, 2024
@TomNicholas
Copy link
Contributor Author

roms-tools looks good, so lets just copy the configuration from there.

@dafyddstephenson
Copy link
Contributor

dafyddstephenson commented Sep 14, 2024

There is now a separate CI section for pre-commit! (somewhere in the PR line). Also we found out this week roms-tools isn't using mypy, so C-Star linting is stricter.

@dafyddstephenson
Copy link
Contributor

There is now a separate CI section for pre-commit! (somewhere in the PR line).

Strike that, it's already in. In that case I'm not sure I understand the issue.

@TomNicholas
Copy link
Contributor Author

TomNicholas commented Sep 14, 2024

In #71 (comment) I pulled down your PR branch, which was passing all green in the CI. I immediately ran pre-commit on it locally, and ruff found a bunch of formatting to fix (applied in 9442114). This shouldn't have been possible if everything is working correctly - the PR should have had a red failure. Beyond that I'm not exactly sure what is wrong yet.

@TomNicholas
Copy link
Contributor Author

There is now a separate CI section for pre-commit! (somewhere in the PR line).

Strike that, it's already in. In that case I'm not sure I understand the issue.

I want it to look more like this, with an explicit extra CI job just for pre-commit (and ideally another one for mypy)

Screenshot 2024-09-14 at 12 05 22 AM

@NoraLoose NoraLoose mentioned this issue Sep 16, 2024
2 tasks
@NoraLoose NoraLoose self-assigned this Sep 16, 2024
@TomNicholas
Copy link
Contributor Author

#81 is an example of what I'm talking about - the CI makes it look like tests are failing, when it's actually just a mypy error (much less of a big deal).

@NoraLoose
Copy link
Contributor

That is annoying. Let me issue a quick PR that separates out the pre-commit stuff from the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration or tooling
Projects
None yet
Development

No branches or pull requests

3 participants