Skip to content

Start linting tuf/exceptions.py with all linters #1721

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

Closed
wants to merge 3 commits into from

Conversation

MVrachev
Copy link
Collaborator

Description of the changes being introduced by the pull request:

We are already using tuf/exceptions.py inside

  • tuf/api/metadata.py
  • tuf/ngclient/__internal__/trusted_metadata_set.py
  • tuf/ngclient/fetcher.py
  • tuf/ngclient/updater.py
  • and in our tests using the new code base

This means that we are most likely going to use that
module after we remove the old code.

It makes sense to lint it with the other three
linters besides mypy (we already lint tuf/exceptions.py with mypy).

Please verify and check that the pull request fulfills the following
requirements
:

  • The code follows the Code Style Guidelines
  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

We are already using tuf/exceptions.py inside
- tuf/api/metadata.py
- tuf/ngclient/__internal__/trusted_metadata_set.py
- tuf/ngclient/fetcher.py
- tuf/ngclient/updater.py
and in our tests that this means we are most likely going to use that
module after we remove the old code.

It make sense to lint it with the other three
linters besides mypy (we already lint tuf/exceptions.py with mypy).

Signed-off-by: Martin Vrachev <[email protected]>
There was one difficulty with pylint that adding tuf/exceptions.py to
the list of ignores didn't work.
That's why I found a different way to disable all pylint warnings with
the "--disables=W" option.

Signed-off-by: Martin Vrachev <[email protected]>
@coveralls
Copy link

coveralls commented Dec 10, 2021

Pull Request Test Coverage Report for Build 1564745748

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 39 of 50 (78.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 98.383%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tuf/exceptions.py 39 50 78.0%
Totals Coverage Status
Change from base Build 1563115608: 0.8%
Covered Lines: 4008
Relevant Lines: 4046

💛 - Coveralls

Copy link
Contributor

@kairoaraujo kairoaraujo left a comment

Choose a reason for hiding this comment

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

We have some commented code as 'Directly instance-repoducing'. Any reason to keep it?

@jku
Copy link
Member

jku commented Dec 13, 2021

Alternatively

  • copy exceptions.py to wherever it should live in the future (tuf/api/exceptions.py maybe?)
  • remove all of the unused exceptions and cruft from that copy
  • make the code actually follow the style guide (and not just pass the linter)
  • apply all the fixes you did here
  • start importing the new copy in api & ngclient

I'm not saying this needs to all happen in one PR, but if you start with making the copy and removing old code, then there's far less code to fix/review and you don't need to deal with pylint disables: the way the PR handles disables almost certainly disabled warnings from all of the old code base, not just exceptions.py

@MVrachev
Copy link
Collaborator Author

Alternatively

  • copy exceptions.py to wherever it should live in the future (tuf/api/exceptions.py maybe?)
  • remove all of the unused exceptions and cruft from that copy
  • make the code actually follow the style guide (and not just pass the linter)
  • apply all the fixes you did here
  • start importing the new copy in api & ngclient

I'm not saying this needs to all happen in one PR, but if you start with making the copy and removing old code, then there's far less code to fix/review and you don't need to deal with pylint disables: the way the PR handles disables almost certainly disabled warnings from all of the old code base, not just exceptions.py

Yes, I can create a new module in tuf/api/exceptions.py and copy the relevant exceptions and follow all the steps.
Will do that in the coming days, but it will be as a side item as I have additional goals for the sprint.

@MVrachev
Copy link
Collaborator Author

This pr is superseded by #1725 where I actually add a new exceptions.py file as @jku suggested.

@MVrachev MVrachev closed this Dec 13, 2021
@MVrachev MVrachev deleted the lint-exceptions branch December 20, 2021 11:51
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.

4 participants