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

BUG: Cope with missing /I in articles #2134

Merged
merged 5 commits into from
Sep 3, 2023
Merged

Conversation

pubpub-zz
Copy link
Collaborator

closes #2089

@MartinThoma MartinThoma changed the title BUG : cope with missing I in articles BUG: Cope with missing I in articles Aug 31, 2023
@MartinThoma MartinThoma changed the title BUG: Cope with missing I in articles BUG: Cope with missing /I in articles Aug 31, 2023
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (af41173) 94.13% compared to head (4aed127) 94.13%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2134   +/-   ##
=======================================
  Coverage   94.13%   94.13%           
=======================================
  Files          41       41           
  Lines        7467     7467           
  Branches     1474     1474           
=======================================
  Hits         7029     7029           
  Misses        273      273           
  Partials      165      165           
Files Changed Coverage Δ
pypdf/_writer.py 87.84% <ø> (ø)

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

@pubpub-zz
Copy link
Collaborator Author

@MartinThoma
the test has failed because of the timeout in test_watermarking_speed whereas it passed just before => This PR can be merged safely, however I'm wondering if we should not rewrite this test🫤

@stefan6419846
Copy link
Collaborator

stefan6419846 commented Aug 31, 2023

Maybe a timeout of 4 seconds is just too tight ...

@pubpub-zz
Copy link
Collaborator Author

Maybe a timeout of 4 seconds is just too tight ...

Your comment identifying the loading as a possible source of discrepancy sounds reasonable. Measuring the time of the water_marking may be more reliable

@stefan6419846
Copy link
Collaborator

When measuring the time of the actual process, you might still want a general test method timeout to stop on too long watermarking time.

@pubpub-zz
Copy link
Collaborator Author

When measuring the time of the actual process, you might still want a general test method timeout to stop on too long watermarking time.

the code is not currently failing in infinite loop but you just want to confirm the acceleration is still present. That's why I thing measuring time just around the merge will be more consistent 🙂

@MartinThoma
Copy link
Member

#2143 was done to help with the flaky tests. Please let me know when/if that occurs again. I know that flaky tests are super annoying and I want to ensure that improving pypdf is as fun as possible :-)

@MartinThoma MartinThoma added the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Sep 3, 2023
@MartinThoma MartinThoma merged commit 3acee1b into py-pdf:main Sep 3, 2023
14 checks passed
@MartinThoma MartinThoma removed the soon PRs that are almost ready to be merged, issues that get solved pretty soon label Sep 3, 2023
MartinThoma added a commit that referenced this pull request Sep 3, 2023
## What's new

### Bug Fixes (BUG)
-  Cope with missing /I in articles (#2134)
-  Fix image look-up table in EncodedStreamObject (#2128)
-  remove_images not operating in sub level forms (#2133)

### Robustness (ROB)
-  Cope with damaged PDF (#2129)

### Documentation (DOC)
-  How to take ownership (#2123)

### Developer Experience (DEV)
-  Download PDFs before executing the tests to not run into timeouts (#2143)
-  Add workflow_dispatch to CI (#2145)
-  Automatically create release message / tag message (#2127)
-  Ensure the REL commit message is consistently created (#2126)

### Testing (TST)
-  Add test for correct rendering of watermarks (#2130)

[Full Changelog](3.15.4...3.15.5)
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.

KeyError: '/I'
3 participants