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

Adjust the advice given for running linter on CI #214

Open
thomaskileyukaea opened this issue Jul 26, 2023 · 1 comment
Open

Adjust the advice given for running linter on CI #214

thomaskileyukaea opened this issue Jul 26, 2023 · 1 comment
Assignees

Comments

@thomaskileyukaea
Copy link
Contributor

Currently the course recommends using a --fail-under score for the linting:

Note we need to add --fail-under=0 otherwise the builds will fail if we don’t get a ‘perfect’ score of 10! This seems unlikely, so let’s be more pessimistic.

I would instead recommend having a file with pre-existing linting errors, and then adopting a zero tolerance approach to new warnings.

This would also require adding instructions for how to suppress specific warnings (# noqa) as sometimes it is legitimate to ignore a warning.

This is better than having a score as:

  • the score allows new things to fall in, which reduces how useful the linter is
  • it actually discourages removing code since you can get into a situation where you want to delete some redundant code, but that actually increases the fraction of code that has a warning, and so your "delete only" PR will fail CI.

(I intend to address this, raising issue for visibility and in case anyone disagrees).

@thomaskileyukaea
Copy link
Contributor Author

The slight problem with this is currently the course advises using pylint, which produces quite a lot of noise. For example, demanding docs on every function, requiring classes to have a minimum of two public methods, banning all single letter variable names etc. I would suggest using flake8, which produces a much less opinionated set of checks (I believe a human is better placed to judge whether variable names are long enough, whether there should be docs etc.)

However, as it stands I wouldn't advise doing this as you'll end up polluting the codebase with endless comments suppressing warnings

We could suggest using flake8, or how to disable the more annoying warnings from pylint. My preference would be the former.

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

No branches or pull requests

2 participants