prithviulm / qudi-core

A framework for modular measurement applications.
https://qudi-core-testing.readthedocs.io/en/latest/
GNU General Public License v3.0
0 stars 2 forks source link

Use a formatter and linter [New Feature] #8

Closed prithviulm closed 1 month ago

prithviulm commented 7 months ago

Feature Description

Add a formatter and linter to the testing process.

Related Problem

No response

Considered Alternatives

Well we could do every-dev-for-themselves but that anarchy would not be good for readability of the code.

Additional Context

No response

Contact Details

No response

prithviulm commented 7 months ago

ruff looks like a great option. It's extremely fast, well-documented, and many big projects use it too. It's also both a linter and formatter. Am currently testing it locally.

prithviulm commented 7 months ago

Most of the remaining linting errors are for bare exceptions, see ea7f8542ed0de75d99b754c6d0687d74e3942544. Need to discuss how best to fix this since I'm not too familiar with the internals. My simple fix now is to just catch the Exception and log it.

prithviulm commented 7 months ago

I ran ruff check . --ignore=E722 to ignore the errors about bare exceptions. There are 5 remaining errors.

src/qudi/core/gui/main_gui/main_gui.py:409:13: F841 Local variable `process` is assigned to but never used
src/qudi/core/parentpoller.py:101:48: F401 `_winapi.INFINITE` imported but unused; consider using `importlib.util.find_spec` to test for availability
src/qudi/tools/config_editor/config_editor.py:23:1: F822 Undefined name `ConfigurationEditorMainWindow` in `__all__`
src/qudi/util/fit_models/linear.py:66:9: F811 Redefinition of unused `estimate_no_offset` from line 60
src/qudi/util/mapper.py:346:9: F841 Local variable `mapping` is assigned to but never used
Found 5 errors.
No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option).

I don't want to address these yet because I don't know if some behind-the-scenes magic is somehow depending on these. Yet another reason to have unit tests, see #10.

prithviulm commented 7 months ago

I ran ruff format . in a8eda8a436d7142127ea4bd8eefd8e6cef1e9d9e. I hope nothing broke but it should be fine since it's just a formatter. Next would be to start working on unit tests in #10 since that's the next big thing left to do. There's a ton of work to do on formatting, linting and documentation but I think it's better to have a framework for the entire CI/CD first and then work on making everything work.

prithviulm commented 7 months ago

Another thing to decide is whether to have linting and formatting as a CI/CD step or a pre-commit hook. I think it's usually been in CI/CD (I could be wrong) because it used to take so long to run, but now ruff is so fast that a lot of people seem to be able to include it as a pre-commit hook. I think that makes sense too since it's a lot neater if people just commit linted and formatted code.

Of course, if one is in a hurry to push a commit and doesn't want to deal with linting errors, they can (I think) bypass the pre-commit hook using the --no-verify flag in git. Maybe some bot could then open an issue and yell at the person daily until they fix it.

prithviulm commented 1 month ago

Seems to be appropriate. Just need to decide on a few common changes to the standards (e.g. max line length, double vs single quotes) with the rest of the Qudi team later.