Linting and code formatting

[stefsmeets]

Hi all, I'm wondering what is your opinion on code formatting and linting. I think PROTEUS and some of the other submodules would really benefit from some linting.

• [x] Import sorting: I noticed the code has a lot of unused imports, and the imports could be sorted according to the pep8 standard (https://peps.python.org/pep-0008/#imports). Linked to #136 -> #138
• [x] Linting: This can help find a bunch of issues with the code, like unused variables, bad practices and anti-patterns, and other potential issues -> #146
• [ ] Auto-formatting: This helps unify the code style from different contributors, in line with the python style guide (https://peps.python.org/pep-0008/).
• [ ] Type checking: If we start adding types, then it will be useful to start adding type checks (https://mypy.readthedocs.io/)

I personally find ruff to be a great tool for linting, formatting, and import sorting. Let me know what you think.

[timlichtenberg]

Sounds useful, I would suggest to discuss this at the next PROTEUS meeting on Monday; it would be good if you could describe a bit what steps and changes this would induce.

[nichollsh]

I agree that we need to have something like this going forward, as more people are working on the code. I am particularly a fan of implementing more typed code.

A maximum line length also makes sense, but may I also suggest a larger limit of 92 chars rather than 79?

[nichollsh]

In addition, we should discuss what kind of naming style is appropriate for variables, functions, etc. The PEP doesn't specify a single style in particular.

The predominant style in the current code is CamelCase, with a few exceptions.

[stefsmeets]

The standard is CamelCase for classes and snake_case for methods, functions, and variables. Globals in UPPERCASE. See: https://peps.python.org/pep-0008/#descriptive-naming-styles

And +1 for 92 or perhaps even larger for line length.

[stefsmeets]

Current state of the code:

❯ ruff check src/* tests/* --statistics --select F,E,W,I
118 E501    [ ] line-too-long
 39 F821    [ ] undefined-name
 36 W291    [*] trailing-whitespace
 24 E701    [ ] multiple-statements-on-one-line-colon
 15 F841    [*] unused-variable
 10 E711    [*] none-comparison
  8 E702    [ ] multiple-statements-on-one-line-semicolon
  6 E741    [ ] ambiguous-variable-name
  5 W293    [*] blank-line-with-whitespace
  3 E721    [ ] type-comparison
  3 E731    [*] lambda-assignment
  2 E712    [*] true-false-comparison
  1 E713    [*] not-in-test
  1 F601    [*] multi-value-repeated-key-literal

Undefined name is somewhat worrying, but seems to be a side-effect of #138

[stefsmeets]

Hi all, just noticed that I pushed a bunch of commits to master by accident that addresses this issue, just so you know. I can't undo it because of branch protection, but I will fix remaining issues next week. Sorry about that.

[timlichtenberg]

How did this work in the first place?

[stefsmeets]

No idea, I forgot to ceate a branch so I committed to master (locally) instead and pushed that 😅 It also struck me that it's kind of odd that the branch protection does not protect against that. I tried to undo and force push the previous state, but that actually is forbidden. Best I can do is revert the changes in a new commit and then push that. Just messes up the history a bit.

[stefsmeets]

https://stackoverflow.com/questions/58541814/how-to-protect-branch-with-require-pull-request-reviews-before-merging/58542669#58542669

Do we have the setting 'Do not allow bypassing' turned on?

[nichollsh]

It is turned off. Should we turn it on?

[timlichtenberg]

Good for me now that every repo has 4 admins.