Skip to content

Add basic linting#2561

Draft
J4bbi wants to merge 1 commit intodevelopfrom
feature/4328_linter
Draft

Add basic linting#2561
J4bbi wants to merge 1 commit intodevelopfrom
feature/4328_linter

Conversation

@J4bbi
Copy link
Copy Markdown
Contributor

@J4bbi J4bbi commented Mar 7, 2026


Add bare minimum linter

A linter checks for certain basic problems in code and a formatter can enforce those rules. These are some of the benefits of having a linter:

  • catch basic runtime bugs (Undefined names (F821), f-strings with no placeholders (F541), invalid escape sequences (W605))
  • consistent code style, we should be able to agree on basic rules, this makes the code more readable and more maintainable
  • more thorough review, less falls through the cracks of human limitations
  • less time spent debugging, things picked up automatically

It's difficult to think of proper downsides to using a linter, other than the bother of having to run it before committing. Mentally that can be a bothersome step, that last bit of housekeeping that throws up tiny little problems as the last hurdle to clear.

Importantly, we should be able to agree on basic linting rules that makes our lives easier. It shouldn't be a question of whether we should integrate a linter, but how.

Linter configuration

The linter is configured in ruff.toml, the following explains the couple of lines in there.

[lint] — configures ruff's linter (as opposed to its formatter).                                                                                                                                                    
                                                                                                                                                                                                                      
  - select = ["E9", "F"] — the only two rule groups enabled:                                                                                                                                                          
    - E9 — pycodestyle syntax errors (e.g. E901 invalid syntax, E902 IO error reading a file). These are show-stoppers that mean the file can't even be parsed.                                                       
    - F — the full pyflakes ruleset. Pyflakes checks for logical problems without caring about style:                                                                                                                 
        - F401 unused imports                                                                                                                                                                                         
      - F403/F405 star imports and names that come from them
      - F811 redefined name
      - F821 undefined name
      - F841 unused variables
      - F541 f-strings with no {} placeholders
      - etc.

Everything else (line length, whitespace, naming conventions, complexity) is ignored.

  [lint.per-file-ignores] — overrides per path pattern.

  - "doajtest/**" = ["F401", "F841"]

in test files, unused imports (F401) and unused variables (F841) are suppressed. Both are common and intentional in tests: imports are often pulled in to trigger side-effects or make fixtures available, and variables like result = some_call() are assigned just to assert they don't raise.

How to install and use

It's listed as a dev dependency. To install, run:

pip install -e .[test]

To use ruff, run:

ruff check

Current results

Linting for E9 and F on the codebase currently results in:

Found 376 errors.
[*] 180 fixable with the `--fix` option (28 hidden fixes can be enabled with the `--unsafe-fixes` option).

This PR...

  • has scripts to run
  • has migrations to run
  • adds new infrastructure
  • changes the CI pipeline
  • affects the public site
  • affects the editorial area
  • affects the publisher area
  • affects the monitoring

Developer Checklist

Developers should review and confirm each of these items before requesting review

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Reviewer Checklist

Reviewers should review and confirm each of these items before approval
If there are multiple reviewers, this section should be duplicated for each reviewer

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Testing

List user test scripts that need to be run

List any non-unit test scripts that need to be run by reviewers

Deployment

What deployment considerations are there? (delete any sections you don't need)

Configuration changes

What configuration changes are included in this PR, and do we need to set specific values for production

Scripts

What scripts need to be run from the PR (e.g. if this is a report generating feature), and when (once, regularly, etc).

Migrations

What migrations need to be run to deploy this

Monitoring

What additional monitoring is required of the application as a result of this feature

New Infrastructure

What new infrastructure does this PR require (e.g. new services that need to run on the back-end).

Continuous Integration

What CI changes are required for this

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.

1 participant