py-pdf / pypdf

A pure-python PDF library capable of splitting, merging, cropping, and transforming the pages of PDF files
https://pypdf.readthedocs.io/en/latest/
Other
8.41k stars 1.42k forks source link

DEV: make make_release.py compatible with windows environment #2894

Closed pubpub-zz closed 1 month ago

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.44%. Comparing base (e14b991) to head (bcbf5db). Report is 3 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2894 +/- ## ========================================== + Coverage 96.43% 96.44% +0.01% ========================================== Files 52 52 Lines 8726 8726 Branches 1721 1589 -132 ========================================== + Hits 8415 8416 +1 Misses 182 182 + Partials 129 128 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pubpub-zz commented 1 month ago

the doc compilation failed randomly. This should be OK

stefan6419846 commented 1 month ago

Could you please add a test to cover the parse_commit_line change accordingly?

pubpub-zz commented 1 month ago

I have no client on how to test a program file not within tests folder

stefan6419846 commented 1 month ago

There is nothing special about this, see the existing test: https://github.com/py-pdf/pypdf/blob/main/tests/scripts/test_make_release.py

pubpub-zz commented 1 month ago

I just gave it a shot - works for me

(except parse_commit_line where I had to replace line 360 by author_login = authors.get(commit_hash, author), but that is not related to the PR)

Can you clarify what error did you get and what was the commit_hash value ?

stefan6419846 commented 1 month ago

I would still prefer to see a test added here.

(except parse_commit_line where I had to replace line 360 by author_login = authors.get(commit_hash, author), but that is not related to the PR)

Which indicates that an author could not be mapped. In theory, this should not occur, although there might be edge cases. Nevertheless, this should probably be fixed and tested as well.

pubpub-zz commented 1 month ago

I would still prefer to see a test added here.

Agréé. in progress

Which indicates that an author could not be mapped. In theory, this should not occur, although there might be edge cases. Nevertheless, this should probably be fixed and tested as well.

Agree. I had a similar issue this is why I completed the strip() waiting for feedback from @MartinThoma

MartinThoma commented 1 month ago

It was a key-not-found error - the the hash was not in the dictionary. I haven't looked more into it.

pubpub-zz commented 1 month ago

@MartinThoma I've been able to reproduce the key not found error, running the script from a branch different to main. Can you confirm it was the same for you

stefan6419846 commented 1 month ago

I've been able to reproduce the key not found error, running the script from a branch different to main.

This is not surprising as this script is only intended for the default branch. Running on other branches would require different GitHub API requests.

pubpub-zz commented 1 month ago

This is not surprising as this script is only intended for the default branch. Running on other branches would require different GitHub API requests.

I agree this is not the objective of this script. I've added the test based on what I've internally objerved in windows. Ready for review

stefan6419846 commented 1 month ago

While the Windows CI breaks because of broken test file downloads, merging is not possible for now. We should probably wait some time before re-running it to avoid rate limit errors.

pubpub-zz commented 1 month ago

I never understood why the files seem to be properly cached in ubuntu but not in windows any ideas?

stefan6419846 commented 1 month ago

Windows does not use the cache as it is not configured there.

pubpub-zz commented 1 month ago

Windows does not use the cache as it is not configured there.

shouldn't we set it up ?

stefan6419846 commented 1 month ago

At the moment, this helps detecting invalid links which are not available any more. Thus I am not completely sure about this.