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

CI: add ruff linting job, fix PEP8/257 violations and formatting #615

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

deltragon
Copy link
Collaborator

@deltragon deltragon commented Jul 25, 2024

CC #589

There should be no functional changes in the code, only formatting.

Note: I originally used flake8+black here, however I switched to ruff as it is a single tool that can check more than the other two (eg. docstrings).

Note: Once this is merged, we should probably add a step for installing/running ruff to the wiki page: https://github.com/slgobinath/SafeEyes/wiki/Run-from-Source

@deltragon
Copy link
Collaborator Author

Well, also looks like the Github Action is working.

@deltragon
Copy link
Collaborator Author

Note to self: Apparently black also has a GitHub Action, so maybe use that instead/in addition.

@nifadyev
Copy link
Contributor

Hey @deltragon , I suppose we should wait for @archisman-panigrahi opinion first. Then we'd better discuss and agree on linter and linter rules
It is awesome that you have already made some changes but personally preferred linting rules is not much better than no rules at all

@nifadyev nifadyev self-requested a review July 26, 2024 07:32
@deltragon
Copy link
Collaborator Author

@nifadyev I agree, I would definitely wait for sign-off from all maintainers here.
In my experience, it is helpful for the discussion to have a specific proposal started, and implementing this didn't take too long - most of this is automatic.

Regarding the chosen style - it seems that the Python community in general has standardized around PEP8 and PEP257, and the official black formatter, which this PR implements. (It uses ruff instead of black/flake8/etc, as it is a single tool that can check all the rules, instead of needing multiple tools - but the rules/formatting stay the same.)

@deltragon deltragon changed the title CI: add flake8 job, fix flake8 violations CI: add ruff linting job, fix PEP8/257 violations and formatting Jul 26, 2024
@archisman-panigrahi
Copy link
Collaborator

I am traveling, and didn't follow any conversation since last weekend. I will get back this sunday

@nifadyev
Copy link
Contributor

nifadyev commented Jul 26, 2024

it is helpful for the discussion to have a specific proposal started

Good point, @deltragon , especially if it does took much time

I will leave comments with suggested linting rules if you don't mind

@deltragon
Copy link
Collaborator Author

@nifadyev Feel free to, I'd appreciate them! I don't have strong opinions to be honest, I just want it to be consistent (and auto-enforced).

"""
Stop Safe Eyes if it is running.
"""
"""Stop Safe Eyes if it is running."""
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, such docstrings are not helpful and could be removed or partially placed into method name. This could be a point in our code styleguide

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. Are there linter rules that we could use that would help with that?
As with the other ones, I would leave this for a future PR as well.

@nifadyev
Copy link
Contributor

Looks great @deltragon , much better than now. Still a lot to do though.

@deltragon
Copy link
Collaborator Author

@nifadyev In general, these are good suggestions.
However, they are mostly purely additive to the changes in this PR, and can be added in a later PR that implements extra rules. I would prefer that they not hinder this PR from being merged.

Copy link
Contributor

@nifadyev nifadyev left a comment

Choose a reason for hiding this comment

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

LGTM @deltragon

Thank you for this important task, I hope the codebase will be much cleaner and single styled now. Agreed about changes, they shall be made in separate PR. Maybe create an issue or some Markdown document with changes to make (or discuss)?

@deltragon deltragon mentioned this pull request Aug 25, 2024
4 tasks
@deltragon
Copy link
Collaborator Author

@nifadyev I've opened #642 to track the future improvements.

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.

3 participants