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

Revise pylint configuration (and linter choice) #1178

Closed
lukpueh opened this issue Oct 15, 2020 · 4 comments
Closed

Revise pylint configuration (and linter choice) #1178

lukpueh opened this issue Oct 15, 2020 · 4 comments
Labels

Comments

@lukpueh
Copy link
Member

lukpueh commented Oct 15, 2020

Description of issue or feature request:
TUF uses a pylint config file that seems to have been copied in verbatim from PyCQA/pylint, listing all defaults explicitly, and adding only a few customizations, which neither fully encode our current style guidelines, nor the style guidelines we plan to adopt (~TBD) for new code.

Note: The same issue exists in securesystemslib, and existed but was fixed in in-toto.

Current behavior:

  • pylintrc unnecessarily lists defaults making it hard to read/maintain
  • pylintrc does not encode our code style guidelines
  • (travis/tox automated testing does not run pylint over test code)

Expected behavior:

  1. Decide on style guidelines (Decide on style guideline #1128)
  2. Encode chosen guidelines in a new pylintrc-file (see tuf/api: Expose tuf.api as a package (take 2) #1177 for WIP preparatory work)
  3. Use new pylintrc for new code including tests
    Note: It is probably not worth fixing the potentially many linter issues in code that might not survive the upcoming refactor.
  4. (orthogonal) review and adopt other code quality tools (e.g. @trishankatdatadog has positively mentioned black and lgtm).
@joshuagl
Copy link
Member

joshuagl commented Mar 3, 2021

#1294 suggests our current Tox configuration is not correctly linting the new code (in tuf/api/), even though we have a Pylint statement in Tox and a minimal configuration file for the new style.

@rzr
Copy link
Contributor

rzr commented Mar 3, 2021

tox is linting the code but I noticed that different pylint versions produce different warnings with current rc file.
Maybe some params can be added to prevent this kind of ambiguity.

I'll comment #1294

now back to this ticket, It made sense to align to default configuration, now if there is a need to enforce custom ones , may I suggest that we progressively pick and adapt relevant lines from generated template:
pylint --generate-rcfile > tuf/api/pylintrc

@lukpueh
Copy link
Member Author

lukpueh commented Mar 8, 2021

Thanks for chiming in, @rzr!

may I suggest that we progressively pick and adapt relevant lines from generated template:
pylint --generate-rcfile > tuf/api/pylintrc

Yes, that's a great idea! Same should go for adopting the rules in the code. See e.g. https://github.com/in-toto/in-toto/pull/296/commits, where I grouped changes by rule and commit, which IMO makes review a lot easier.

Regarding #1294 (comment) ...

Any objection to use black for formatting ?:

No, not at all! :) I have planned to work on this in the near future, but I'd definitely appreciate help.

(cc @fepitre + in-toto/apt-transport-in-toto#36 (comment))

Please note that both revised linter and auto-formatter config should align with our new python style guide.

@sechkova sechkova added the backlog Issues to address with priority for current development goals label May 26, 2021
@sechkova sechkova modified the milestones: Foundations, weeks22-23 May 26, 2021
@joshuagl joshuagl modified the milestones: weeks22-23, Foundations May 26, 2021
@joshuagl joshuagl removed this from the weeks22-23 milestone Jun 9, 2021
@joshuagl joshuagl removed the backlog Issues to address with priority for current development goals label Jul 28, 2021
@lukpueh
Copy link
Member Author

lukpueh commented Feb 17, 2022

Fixed in #1314 and follow up PRs, we now use a minimally configured pytlint, black and isort for the whole repo (including tests).

@lukpueh lukpueh closed this as completed Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants