NeuroBench / neurobench

Benchmark harness and baseline results for the NeuroBench algorithm track.
https://neurobench.readthedocs.io
Apache License 2.0
46 stars 11 forks source link

Code formatting / linting #187

Closed jasonlyik closed 3 months ago

jasonlyik commented 5 months ago

Should add some code formatter and linter to make the whole repo more consistent

cerrussell commented 5 months ago

@jasonlyik Black, pylint, flake8 are helpful for this. Pre-commit needs to be used to actually enforce, however, and the entire project would need to be formatted and linted at the start. Though there are ways you could do automatic code formatting via a workflow, I really would not recommend that approach as no tool is perfect and there will be times when a tool does something undesirable. For example, black does not handle string literals well at all when it addresses maximum line length. Specifically, it likes to add additional quotation marks so that the line is longer than ever without splitting the content up across multiple lines. Black also won't format your docstrings or comments, and I'm not aware of a tool that does.

Pre-commit with black, pylint, and flake8, would be my rec - here's an example of a repo using pre-commit with the second two (I removed black because of the above issue, but would try using it first).

I can help set this up if you like.

jasonlyik commented 5 months ago

@cerrussell Thanks! It would be great if you could help to set this up. We haven't had an enforced style throughout our codebase, which tools would be useful to update the whole thing to a consistent style? Also do you have any sense of how difficult that would be?

cerrussell commented 5 months ago

@jasonlyik It would not be difficult - there are two separate PRs I would propose. The second step of PR 2 would be optional, but I would recommend not skipping as they are such simple fixes. Otherwise those items will need to be added to the exclusions in step 3.

PR 1

  1. Run pylint and flake8 and fix the complaints related to formatting that do not have an effect on the meaning of the code (things like line length, missing or inappropriate whitespace, missing or inappropriately placed docstrings, unneeded parentheses, etc.)
  2. I would use the black integration in PyCharm to format each file, make sure nothing undesirable was done, then manually address any lines which exceed the desired line length which PyCharm/black could not fix automatically.

PR 2

  1. Add the .pre-commit-config.yaml and pre-commit workflow.
  2. Address non-formatting items that are straightforward to fix, such as (non-exhaustive):
    • unused arguments
    • attribute-defined-outside-init
    • no-else-return or no-else-continue
    • dangerous-default-value These things technically change the code but require minimal change and are very unlikely to have unforeseen consequences.
  3. Finally, look at the items that change the code (like too many arguments/attributes/locals) and adjust the pre-commit or pylint and flake8 configuration settings. I would definitely suggest seeing if some refactoring could address some of these, but that's not what this issue is about, so I would only be adding exclusions and/or noqa annotations to suppress non-formatting related warnings so that pre-commit will pass as intended.
jasonlyik commented 3 months ago

196