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

Type annotate bandersnatch plugins #561

Merged
merged 10 commits into from
Jun 8, 2020
Merged

Conversation

ichard26
Copy link
Member

@ichard26 ichard26 commented Jun 8, 2020

Related: #15.

Type annotate the bandersnatch plugins.

@techalchemy, it would be nice if you could review this PR since you know these plugins the best. (Only if you have the time though)

@ichard26 ichard26 changed the title Type plugins Type annotate bandersnatch plugins Jun 8, 2020
@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #561 into master will increase coverage by 0.12%.
The diff coverage is 86.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #561      +/-   ##
==========================================
+ Coverage   78.77%   78.89%   +0.12%     
==========================================
  Files          13       13              
  Lines        2035     2033       -2     
  Branches      289      289              
==========================================
+ Hits         1603     1604       +1     
+ Misses        326      324       -2     
+ Partials      106      105       -1     
Impacted Files Coverage Δ
src/bandersnatch_storage_plugins/swift.py 70.76% <85.10%> (+0.44%) ⬆️
src/bandersnatch_storage_plugins/filesystem.py 81.41% <100.00%> (ø)

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 fd96e10...d5b1ef4. Read the comment docs.

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.

Thanks - Looks legit ... TIL ignore[override] ...

Can you please unblacklist the plugins directory on pre-commit here now please to get MyPy testing it: https://github.com/pypa/bandersnatch/blob/master/.pre-commit-config.yaml#L33 on CI

@ichard26
Copy link
Member Author

ichard26 commented Jun 8, 2020

I also learned # type: ignore[override] today. I was a little annoyed with Signature incompatible with Super type warnings. Also, trust me, the only reason why it looks legit is because of the amount of time I spent on it.

@cooperlees
Copy link
Contributor

And I appreciate it. As someone who’s typed a lot of old code, I appreciate the difficulties. Typing code changes how you code moving forward. For the better I believe :)

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.

CI agrees and Thankyou very much :)

"""
Remove all release versions that match any of the specified patterns.
"""
releases = metadata["releases"]
for version in list(releases.keys()):
if any(pattern.match(version) for pattern in self.patterns):
if any(pattern.match(version) for pattern in self.patterns): # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Will be interested to see what the error is here.

Copy link
Member Author

Choose a reason for hiding this comment

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

prerelease_name.py:41: error: Item "None" of "Optional[List[Pattern[str]]]" has no attribute "__iter__" (not iterable)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh - makes sense.

Yeah, here we just need to add another check for a None object and handle it. I’ll go back and look at type ignores before closing the issue and see if we can get rid of a few more.

@cooperlees cooperlees merged commit fa41235 into pypa:master Jun 8, 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