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 filtering of verify and should fix #1488 #1690

Merged
merged 2 commits into from
Dec 24, 2024
Merged

Conversation

bammab
Copy link
Contributor

@bammab bammab commented Mar 23, 2024

Hi,
I run in the same problem as the user of issue #1488 and this PR should fix the bug. Because none of the json files have a version field, I was first looking where this field come from. A review with git blame of verify.py don't helped a lot and I'm not sure, how this code should ever work.

  1. The json content is passed unchanged to the filter, but the Package class create specific release/metadata dicts with other contents/fields - here we could find the "version" field.
  2. The filters are called without any condition on the result. So the check seems to be useless?!

To fix that, I wrapped the json package inside a Package class object and call the same functions as the mirror class do and it seems to work as expected.

Please check my PR. If I had made any mistakes or I don't understand how it should work, I will promise to fix that ASOP.

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 86.14%. Comparing base (4d020e8) to head (732cca0).
Report is 188 commits behind head on main.

Files with missing lines Patch % Lines
src/bandersnatch/verify.py 12.50% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1690      +/-   ##
==========================================
+ Coverage   79.69%   86.14%   +6.44%     
==========================================
  Files          31       33       +2     
  Lines        4324     4395      +71     
  Branches      780      464     -316     
==========================================
+ Hits         3446     3786     +340     
+ Misses        721      431     -290     
- Partials      157      178      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cooperlees
Copy link
Contributor

Hi,

Thanks for the PR. This seems like this should work. I wonder if we could somehow add a test to show verify respecting some simple filter options now you've added support? They would extend here

Thanks!

@cooperlees cooperlees added the skip news Skip CI check for news/changelog label Dec 24, 2024
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.

This seems to work, although using private parts of Package class (maybe we should make a load from metadata method or something if it does not already exist.

Going to merge and add to CHANGES in another commit.

@cooperlees cooperlees merged commit ae40be1 into pypa:main Dec 24, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Skip CI check for news/changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants