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 #3

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

Add ruff #3

wants to merge 22 commits into from

Conversation

jo47011
Copy link
Owner

@jo47011 jo47011 commented Dec 1, 2024

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

Still WIP. Need to adjust docs.

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 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.

There is another issue in the main repo: the midi module seems to be missing for the unittest. Thus 3 tests in test_midi.py are skipped. I did the same in my build.yml as otherwise they would fail. So if we want them to be run in the workflow need to fix them there first. This is from the main repo (e.g. here https://github.com/mhthies/smarthomeconnect/actions/runs/12109227223/job/33758107287?pr=99):
image

@@ -103,7 +103,9 @@ jobs:
- name: Install Python dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements.txt
mkdir _static
Copy link
Owner 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

pip install -r requirements.txt
pip install .[test]
pip install .[mysql,knx,dmx,midi,mqtt,pulse,telegram,file_persistence]
pip uninstall python-rtmidi -y
Copy link
Owner Author

@jo47011 jo47011 Dec 2, 2024

Choose a reason for hiding this comment

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

This is a workaround so some midi tests are skipped as in the main branch of the core repo. See PR description for details.

pyproject.toml Outdated
[tool.ruff.lint]
# Enable checks for Pycodestyle, Pyflakes, Bugbear, Docstrings and Import sorting
select = ["E", "F", "B", "D", "I"]
ignore = [ # FIXME: TBD what we want to ignore, some are easy to auto-fix
Copy link
Owner Author

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.

We should discuss which ones what we want to ignore, some are easy to auto-fix. We can do ti step by step.

pyproject.toml Outdated

[tool.ruff.format]
# Like Black, use double quotes for strings.
# quote-style = "double" # FIXME: do we want this now?
Copy link
Owner Author

Choose a reason for hiding this comment

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

For me it's fine to always have quotes. But we can discuss it once we address formatting in the 2nd stage.

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.

1 participant