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

Fix linter issues where needed #440

Closed
lukpueh opened this issue Oct 21, 2022 · 5 comments · Fixed by #829
Closed

Fix linter issues where needed #440

lukpueh opened this issue Oct 21, 2022 · 5 comments · Fixed by #829
Labels
tests Issues related to testing

Comments

@lukpueh
Copy link
Member

lukpueh commented Oct 21, 2022

blocked on #270, partially addressed by #432

Current behavior:
#439 turned on automatic linting with pylint and bandit, but inline-disabled them for all existing issues, above all to not waste time on code whose future is uncertain (see #270).

Expected behavior:
When resolving #270, we should not forget to address any of the remaining linter issues. They can be found by searching for the following inline code comments:

  • # nosec

  • # pylint: disable=<msg>[, <msg>, ...], where <msg> is one of:

    abstract-method
    bad-classmethod-argument
    broad-except
    consider-using-enumerate
    consider-using-f-string
    consider-using-from-import
    consider-using-with
    dangerous-default-value
    global-statement
    global-variable-not-assigned
    implicit-str-concat
    import-error
    inconsistent-return-statements
    invalid-name
    logging-format-interpolation
    logging-not-lazy
    missing-class-docstring
    missing-function-docstring
    missing-module-docstring
    no-else-raise
    no-else-return
    no-member
    no-name-in-module
    protected-access
    raise-missing-from
    redefined-builtin
    redefined-outer-name
    redundant-unittest-assert
    singleton-comparison
    super-init-not-called
    super-with-arguments
    too-many-branches
    too-many-locals
    too-many-statements
    unnecessary-pass
    unneeded-not
    unspecified-encoding
    unsubscriptable-object
    unused-argument
    unused-import
    unused-variable
    use-implicit-booleaness-not-len
    useless-object-inheritance
    wrong-import-position
    
@lukpueh
Copy link
Member Author

lukpueh commented Oct 21, 2022

Maybe we want to address some of these issues even before #270.

@lukpueh lukpueh added the tests Issues related to testing label Mar 14, 2024
@lukpueh
Copy link
Member Author

lukpueh commented Apr 19, 2024

python-tuf uses ruff for linting and formatting these days and the speedup is veeery appealing. I suggest to switch here too. Now seems a good time, given that most of #270 is addressed.

@L77H
Copy link
Contributor

L77H commented Jun 25, 2024

Hi @lukpueh, are you guys still looking for contributors?
I am looking at getting into OSS contributing, and this issue and related ones like #358 seem to be a good point to start working on. Could you give me any pointers or other open issues that are appropriate to getting started?

@jku
Copy link
Collaborator

jku commented Jun 25, 2024

I think the original details in this issue are a little outdated but lukas's last comment is very much on the table: we've had good experiences with ruff linter in other projects and would definitely accept PRs that would move us to using ruff instead of current pylint+bandit+black+isort.

This can be done in small steps (I would suggest each step to be a PR or several):

  • replace black and isort with ruff format --diff as the code formatting checker (in tox.ini lint section). This will require at least adding ruff into requirements-lint.txt and possibly adding some ruff format config in pyproject.toml
  • add ruff check as just another linting step (in tox.ini lint section). This likely requires some code fixes to comply with ruff rules and may require configuration to skip some rules
  • increase ruff check ruleset coverage so that it's at least as good as our pylint+bandit coverage: in other words add ruff rulesets in ruff config and fix issues that pop up
  • Finally remove bandit and pylint calls from tox.ini lint section

python-tuf is likely a good example off ruff usage. See e.g. how ruff is called in tox.ini: https://github.com/theupdateframework/python-tuf/blob/develop/tox.ini#L51

If you need hints on how to get started with running the linters, this is what I'd do to get started with the first step:

  • install tox
  • run tox -e lint to run the linters just to check they work
  • add ruff into requirements-lint.txt
  • replace black & isort calls with ruff format --diff . in tox.ini
  • see what ruff complains about and try to figure out if it makes sense to add some ruff configuration (like line length) to silence the complaint or to modify the code
  • ask here or in the slack channel if you need advice

358 is also good: it's possible that we're just missing the py.typed file at this point but without looking into it further I'm not sure how to validate that...

@L77H
Copy link
Contributor

L77H commented Jun 25, 2024

@jku Thanks for the pointers! I will start working on replacing the current pylint+bandit+black+isort with ruff and submit them in several PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Issues related to testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants