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

[verify]: Conform to PEP 484 type hints #488

Merged
merged 2 commits into from
Apr 19, 2020
Merged

[verify]: Conform to PEP 484 type hints #488

merged 2 commits into from
Apr 19, 2020

Conversation

SanketDG
Copy link
Contributor

Part of #15

Copy link
Contributor

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

  • Lint mypy.ini wants a new line at the end of the file
  • Seem we need to update the typing for Filter.filter or fix the bug on line 126
src/bandersnatch/verify.py:126: error: Too many arguments for "filter" of "Filter"

It's probably never been hit / tested correctly - Blame me - Feel free to #type: ignore and open an issue and I'll look into it when I next get some time.

config: ConfigParser,
json_file: str,
mirror_base_path: Path,
all_package_files: List,
Copy link
Contributor

Choose a reason for hiding this comment

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

List can take the type in the list - List[Path]

  • The more info you give typing the more it will help us spot bugs etc. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, thanks, I was a little confused on what the sub-type was!

@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #488 into master will increase coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   81.50%   81.51%   +0.01%     
==========================================
  Files          10       10              
  Lines        1157     1158       +1     
  Branches      171      171              
==========================================
+ Hits          943      944       +1     
  Misses        172      172              
  Partials       42       42              
Impacted Files Coverage Δ
src/bandersnatch/verify.py 51.79% <75.00%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd21731...db58728. Read the comment docs.

@SanketDG
Copy link
Contributor Author

Thanks for the quick review, made some changes, opened the PR for review/merge.

@SanketDG SanketDG marked this pull request as ready for review April 19, 2020 20:34
Copy link
Contributor

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks. Will merge.

  • Don't care about the weird coverage reduction I don't get :|

Comment on lines +142 to +143
if __name__ == "__main__":
unittest.main()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pytest these one day to match all other unittests ...



if __name__ == "__main__":
pytest.main(sys.argv)
Copy link
Contributor

Choose a reason for hiding this comment

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

I generally just run pytest src/bandersnatch/tests/test_verify.py to run specific tests ...

@cooperlees cooperlees merged commit 64f9eea into pypa:master Apr 19, 2020
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.

2 participants