Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ruff #100

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Add ruff #100

wants to merge 22 commits into from

Conversation

jo47011
Copy link
Contributor

@jo47011 jo47011 commented Dec 1, 2024

Replace all the original project setup, linting, etc. with ruff.

Taking it step by step. We could have a PR for each step to simplify code reading and discussions.

  1. Ruff: Introduce ruff.
    We disable all linting errors that we get so we can focus on the project setup 1st.

  2. Formatting: We agree on some parameters, see [tool.ruff.format] in pyproject.toml. Then we reformat the code (replaces black):

    ruff format .

    Then we can also add a format check to the build workflow.

  3. Linting: We agree on which parameter we want to enable and which we want to ignore in future, see [tool.ruff.lint] in pyproject.toml, e.g.;

    "D106",  # Missing docstring in public nested class
    "D107",  # Ignore D107 Missing docstring  # FIXME: enable again???
    "D200",  # One-line docstring should fit on one line
    ...

invoke like this:

ruff check --fix .
  1. Build-system: we could consider switching from Setuptools to Hatchling further down the road. Provides for some more advanced environment management and other nice features.

BTW I think there was a bug in your previous setup.cfg:
image

I fixed it in the pyproject.toml. If it was intentional we need to change it back.

We should also discuss the skipping of the tests in test_midi.py. There where skipped because python-rtmidi was not installed but midi was. Does this make sense? I changed the logic slightly and now catch the exception as well if module midi is not loaded. See my comments in the code for details.

@jo47011 jo47011 marked this pull request as draft December 1, 2024 21:27
@jo47011 jo47011 closed this Dec 1, 2024
@jo47011 jo47011 reopened this Dec 4, 2024
@@ -103,7 +100,9 @@ jobs:
- name: Install Python dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements.txt
mkdir _static
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this here to avoid the warning?

WARNING: html_static_path entry '_static' does not exist

[tool.ruff.lint]
# Enable checks for Pycodestyle, Pyflakes, Bugbear, Docstrings and Import sorting
select = ["E", "F", "B", "D", "I"]
ignore = [
Copy link
Contributor Author

@jo47011 jo47011 Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBD all these errors occur, currently disabled so we could merge the current state.
As proposed we can enable and fix them one by one in step 3. Some are easy to auto-fix

@@ -0,0 +1,56 @@
"""Extract the names int the [project.optional-dependencies] section of a pyproject.toml file.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where to put the script. Let me know whether you prefer it somewhere else.

I don't like the need for such a script in the 1st place but there seems to be no generic way like pip install .[all] when using pyproject.toml. Using this scripts avoids repeating dependencies which would be hard to maintain in the long run, e.g.

   pip install .[mysql,knx,dmx,midi,mqtt,pulse,telegram,file_persistence]

So for me the script is the better choice.

Copy link

codecov bot commented Dec 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.84%. Comparing base (f8718f1) to head (19da71e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
- Coverage   83.26%   81.84%   -1.42%     
==========================================
  Files          42       42              
  Lines        5636     5636              
  Branches      768      768              
==========================================
- Hits         4693     4613      -80     
- Misses        741      823      +82     
+ Partials      202      200       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +9 to +12
import mido
import shc.interfaces.midi
from shc.datatypes import RangeUInt8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to move the import here.

Avoids the pip uninstall python-rtmidi to skip those tests. Not sure whether this is really the best logic as this happens quietly and might not be noticed if skipped accidently.

@@ -18,6 +18,7 @@
mido_backend_error = str(e)


@unittest.skipUnless(mido_backend_available, "mido MIDI backend is not available: {}".format(mido_backend_error))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is new to skip this test as well. Other @unittest.skipUnless changes is a typo fix only ("available").

Comment on lines -10 to -12

def test_midi(self):
import shc.interfaces.midi # noqa: F401
Copy link
Contributor Author

@jo47011 jo47011 Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to remove this one. Well it is tested in test_midi.py which is skipped in the pipeline.

Is my current proposal sufficient or do you feel like we need this test here? Then we rollback to your approach and install midi but uninstall python-rtmidi which is kind of confusing to me. Maybe we should split the dependency then.

@jo47011 jo47011 marked this pull request as ready for review December 4, 2024 16:35
@jo47011
Copy link
Contributor Author

jo47011 commented Dec 4, 2024

As for the failed code coverage report, I suggest we fix it once we have the test strategy for the midi stuff clarified.
Can't we just mock the failing midi tests:

_rtmidi.SystemError: MidiInAlsa::initialize: error creating ALSA sequencer client object.

@jo47011
Copy link
Contributor Author

jo47011 commented Dec 11, 2024

Hi @mhthies, don't want to push you (seriously not). Just want to make sure we're on the same page. I would like you to make a 1st review and provide some feedback. I know not all checks are passing right now but the fix would depend on the result of our discussion. Feel assigned as reviewer to this PR 😉 No rush, I'm also quite busy right now.

@mhthies
Copy link
Owner

mhthies commented Dec 16, 2024

Sorry, @jo47011, I'm currently quite busy with work, christmas preparations and another freetime project which is due in the first week of January, so I couldn't take the time for an appropriate review of the changes, yet.

However, thanks already for all the work you put into this PR! :)
Overall, I like your proposed procedure. I also like moving all the tool configuration to the pyproject.toml. But I need to take a closer look and rethink my motivation for the now removed requirements.txt file, as well as for the decision to disable the midi tests via the availability of the midi backend library. There might have been rationals for the existing solutions that are still valid. I also want to take a closer look at the configuration file and script.

Regarding the configuration of ruff's formatting and linting, I'd like to stay as close as possible to the default configuration of the linting tools, unless there are single rules that really annoy me – which I have to check. On the other hand, I'd also like to stick to the Python default-provided tools like Setuptools, to avoid additional dependencies, unless they are able to solve actual problems that otherwise require workarounds.

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.

2 participants