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 all linting commands, even if some of them fail #1882

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

waxlamp
Copy link
Member

@waxlamp waxlamp commented Mar 4, 2024

Without the ignore_errors configuration option set to True, Tox exits as soon as any command in the list fails. In the case of the lint tests, we would like to continue running the commands, since they are essentially independent tests.

This was masking formatting errors whenever the check command was failing, preventing us from knowing that we should fix those.

(attn: @marySalvi)

Without the `ignore_errors` configuration option set to `True`, Tox
exits as soon as any command in the list fails. In the case of the lint
tests, we would like to continue running the commands, since they are
essentially independent tests.

This was masking formatting errors whenever the `check` command was
failing, preventing us from knowing that we should fix those.
Copy link
Member

@mvandenburgh mvandenburgh left a comment

Choose a reason for hiding this comment

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

@brianhelba may also be interested in this, as we move towards integrating Ruff into the girder4/resonant stack.
Docs for this option

@waxlamp
Copy link
Member Author

waxlamp commented Mar 4, 2024

@brianhelba may also be interested in this, as we move towards integrating Ruff into the girder4/resonant stack. Docs for this option

Thanks for mentioning the docs, @mvandenburgh; I forgot to mention that ignore_errors is a bit of a misnomer since the tox invocation still fails if any of the individual commands do (i.e. it's not really ignoring the errors in the end).

Note that some code formatting failures are shown in the CI for this branch; @marySalvi is going to push a fix for that in a separate PR, and I'll go ahead and merge this one now.

@waxlamp waxlamp merged commit b5da025 into master Mar 4, 2024
10 of 11 checks passed
@waxlamp waxlamp deleted the tox-ignore-errors branch March 4, 2024 19:49
@brianhelba
Copy link
Collaborator

@waxlamp @mvandenburgh Makes sense, especially for this one task, which is a read-only check where the steps aren't dependent on each other.

@dandibot
Copy link
Member

dandibot commented Mar 6, 2024

🚀 PR was released in v0.3.78 🚀

@dandibot dandibot added the released This issue/pull request has been released. label Mar 6, 2024
@waxlamp
Copy link
Member Author

waxlamp commented Jun 6, 2024

@brianhelba, is this enhancement on your radar for "upstreaming" to an eventual ruff-based Resonant cookiecutter?

@brianhelba
Copy link
Collaborator

I don't think we'll add Codespell as an upstream recommendation. It seems to have too many false positives.

I don't think it's necessary to run the format checks if the linting fails (since formatting shouldn't be fixed manually), but I'm happy to discuss it more in an upstream PR if someone wants to make it.

@waxlamp
Copy link
Member Author

waxlamp commented Jul 11, 2024

I don't think we'll add Codespell as an upstream recommendation. It seems to have too many false positives.

Oh yes, I didn't mean to include Codespell in this.

I don't think it's necessary to run the format checks if the linting fails (since formatting shouldn't be fixed manually), but I'm happy to discuss it more in an upstream PR if someone wants to make it.

Are you saying that (auto)formatting should only happen once linting passes? Aren't there cases where fixing the formatting would eliminate some (or maybe even all) linting errors?

@yarikoptic
Copy link
Member

I don't think we'll add Codespell as an upstream recommendation. It seems to have too many false positives.

What upstream is the conversation about here?

@waxlamp
Copy link
Member Author

waxlamp commented Jul 12, 2024

I don't think we'll add Codespell as an upstream recommendation. It seems to have too many false positives.

What upstream is the conversation about here?

Resonant (formerly known as Girder 4).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants