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

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

Merged
merged 4 commits into from
Oct 13, 2024

Conversation

pubpub-zz
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@            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     

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

@pubpub-zz
Copy link
Collaborator Author

the doc compilation failed randomly. This should be OK

make_release.py Outdated Show resolved Hide resolved
Co-authored-by: Stefan <96178532+stefan6419846@users.noreply.github.com>
@stefan6419846
Copy link
Collaborator

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

@pubpub-zz
Copy link
Collaborator Author

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

@stefan6419846
Copy link
Collaborator

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

@MartinThoma MartinThoma self-requested a review October 9, 2024 19:24
MartinThoma
MartinThoma previously approved these changes Oct 9, 2024
Copy link
Member

@MartinThoma MartinThoma left a comment

Choose a reason for hiding this comment

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

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)

@pubpub-zz
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Member

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

@pubpub-zz
Copy link
Collaborator Author

@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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@stefan6419846
Copy link
Collaborator

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

@pubpub-zz
Copy link
Collaborator Author

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

shouldn't we set it up ?

@stefan6419846
Copy link
Collaborator

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

@stefan6419846 stefan6419846 merged commit c9dda9a into py-pdf:main Oct 13, 2024
16 checks passed
stefan6419846 added a commit that referenced this pull request Oct 27, 2024
## What's new

### New Features (ENH)
- Add `layout_mode_font_height_weight` argument to `PageObject.extract_text()` (#2920) by @hpierre001

### Bug Fixes (BUG)
- Fix font specificier for FreeText annotation (#2893) by @ssjkamei
- Line breaks are not generated due to incorrect calculation of text leading (#2890) by @ssjkamei
- Improve handling of spaces in text extraction (#2882) by @ssjkamei

### Robustness (ROB)
- Soft failure for flate encode image mode 1 with wrong LUT size (#2900) by @stefan6419846

### Documentation (DOC)
- Use latest package versions (#2907) by @stefan6419846
- Correct example of reading FileAttachment annotation (#2906) by @j-t-1

### Developer Experience (DEV)
- Update pinned requirements (#2918) by @stefan6419846
- Make make_release.py compatible with Windows environment (#2894) by @pubpub-zz

### Maintenance (MAINT)
- Remove references to outdated Python versions (#2919) by @stefan6419846
- Generalize the method of obtaining space_code (#2891) by @ssjkamei
- Unnecessary character mapping process (#2888) by @ssjkamei
- New LZW decoding implementation (#2887) by @MartinThoma

### Testing (TST)
- Add LzwCodec for encoding (#2883) by @MartinThoma

### Code Style (STY)
- Capitalize error messages (#2903) by @j-t-1
- Modify error messages in PdfWriter (#2902) by @j-t-1

[Full Changelog](5.0.1...5.1.0)
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.

3 participants