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.

• 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 External module imports #136 -> Fix * imports #138
• Linting: This can help find a bunch of issues with the code, like unused variables, bad practices and anti-patterns, and other potential issues -> Add linting config and fix linting 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.