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

MAINT: Use grouped constants instead of literals #745

Merged
merged 10 commits into from
Apr 16, 2022
Merged

Conversation

MartinThoma
Copy link
Member

@MartinThoma MartinThoma commented Apr 14, 2022

This allows us to document what they are good for and to distinguish
literals of the same name but of different contexts,
e.g. /Type in Pages or /Type in Page

The constants are in PyPDF2/pdf_attributes.py

This allows us to document what they are good for and to distinguish
literals of the same name but of different contexts,
e.g. /Type in Pages or /Type in Page
@MartinThoma
Copy link
Member Author

@MasterOdin What do you think about this type of change (it's not done jet; at the moment it's more an example how I imagine it).

I see it like this:

  • Linking help texts / context: Having attributes instead of literals allows us to use the editor/IDE to jump to an explanation of the literal. I see this as a massive advantage
  • Readability: We need to be careful that things are still readable. So we shouldn't end up with Java-esque lines where every name is super explicit, but you have a hard time grasping the complete line of code as it's simply too long. That is a disadvantage of this approach, but it could be mitigated by abbreviating the names when importing
  • Runtime: I hope that it doesn't really make a difference for typical workflows. However, we still don't have any performance tests.

@MasterOdin
Copy link
Member

I really like this approach as it makes it harder to have "text misspelling bugs" and as you note it makes it easier to then have reference to relevant documentation on these attributes.

For readability, I think that what you have (alias the long class name to an abbreviation) renders this point largely moot, and that the gain in documentation and not having "magic strings" outweighs this heavily.

Performance is going to be so neglibly affected (if at all really) that I would not worry about it.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2022

Codecov Report

Merging #745 (74094ea) into main (87aafd6) will increase coverage by 0.95%.
The diff coverage is 86.28%.

@@            Coverage Diff             @@
##             main     #745      +/-   ##
==========================================
+ Coverage   69.64%   70.59%   +0.95%     
==========================================
  Files           9       10       +1     
  Lines        3317     3425     +108     
  Branches      783      798      +15     
==========================================
+ Hits         2310     2418     +108     
  Misses        763      763              
  Partials      244      244              
Impacted Files Coverage Δ
PyPDF2/pagerange.py 98.11% <ø> (ø)
PyPDF2/filters.py 58.67% <66.66%> (+0.97%) ⬆️
PyPDF2/generic.py 66.85% <70.37%> (+0.04%) ⬆️
PyPDF2/pdf.py 72.42% <82.45%> (+0.03%) ⬆️
PyPDF2/merger.py 72.56% <83.33%> (+0.09%) ⬆️
PyPDF2/__init__.py 100.00% <100.00%> (ø)
PyPDF2/constants.py 100.00% <100.00%> (ø)
PyPDF2/xmp.py 43.09% <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 87aafd6...74094ea. Read the comment docs.

@MartinThoma MartinThoma merged commit d5a5eea into main Apr 16, 2022
@MartinThoma MartinThoma deleted the use-constants branch April 16, 2022 07:25
MartinThoma added a commit that referenced this pull request Apr 18, 2022
Deprecations (DEP):
-  Remove support for Python 2.6 and older (#776)

New Features (ENH):
-  Extract document permissions (#320)

Bug Fixes (BUG):
-  Clip by trimBox when merging pages, which would otherwise be ignored (#240)
-  Add overwriteWarnings parameter PdfFileMerger (#243)
-  IndexError for getPage() of decryped file (#359)
-  Handle cases where decodeParms is an ArrayObject (#405)
-  Updated PDF fields don't show up when page is written (#412)
-  Set Linked Form Value (#414)
-  Fix zlib -5 error for corrupt files (#603)
-  Fix reading more than last1K for EOF (#642)
-  Acciental import

Robustness (ROB):
-  Allow extra whitespace before "obj" in readObjectHeader (#567)

Documentation (DOC):
-  Link to pdftoc in Sample_Code (#628)
-  Working with annotations (#764)
-  Structure history

Developer Experience (DEV):
-  Add issue templates (#765)
-  Add tool to generate changelog

Maintenance (MAINT):
-  Use grouped constants instead of string literals (#745)
-  Add error module (#768)
-  Use decorators for @staticmethod (#775)
-  Split long functions (#777)

Testing (TST):
-  Run tests in CI once with -OO Flags (#770)
-  Filling out forms (#771)
-  Add tests for Writer (#772)
-  Error cases (#773)
-  Check Error messages (#769)
-  Regression test for issue #88
-  Regression test for issue #327

Code Style (STY):
-  Make variable naming more consistent in tests

All changes: 1.27.5...1.27.6
VictorCarlquist pushed a commit to VictorCarlquist/PyPDF2 that referenced this pull request Apr 29, 2022
Deprecations (DEP):
-  Remove support for Python 2.6 and older (py-pdf#776)

New Features (ENH):
-  Extract document permissions (py-pdf#320)

Bug Fixes (BUG):
-  Clip by trimBox when merging pages, which would otherwise be ignored (py-pdf#240)
-  Add overwriteWarnings parameter PdfFileMerger (py-pdf#243)
-  IndexError for getPage() of decryped file (py-pdf#359)
-  Handle cases where decodeParms is an ArrayObject (py-pdf#405)
-  Updated PDF fields don't show up when page is written (py-pdf#412)
-  Set Linked Form Value (py-pdf#414)
-  Fix zlib -5 error for corrupt files (py-pdf#603)
-  Fix reading more than last1K for EOF (py-pdf#642)
-  Acciental import

Robustness (ROB):
-  Allow extra whitespace before "obj" in readObjectHeader (py-pdf#567)

Documentation (DOC):
-  Link to pdftoc in Sample_Code (py-pdf#628)
-  Working with annotations (py-pdf#764)
-  Structure history

Developer Experience (DEV):
-  Add issue templates (py-pdf#765)
-  Add tool to generate changelog

Maintenance (MAINT):
-  Use grouped constants instead of string literals (py-pdf#745)
-  Add error module (py-pdf#768)
-  Use decorators for @staticmethod (py-pdf#775)
-  Split long functions (py-pdf#777)

Testing (TST):
-  Run tests in CI once with -OO Flags (py-pdf#770)
-  Filling out forms (py-pdf#771)
-  Add tests for Writer (py-pdf#772)
-  Error cases (py-pdf#773)
-  Check Error messages (py-pdf#769)
-  Regression test for issue py-pdf#88
-  Regression test for issue py-pdf#327

Code Style (STY):
-  Make variable naming more consistent in tests

All changes: py-pdf/pypdf@1.27.5...1.27.6
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