Skip to content

MAINT: Deduplicate CatalogAttributes and CatalogDictionary#3868

Open
j-t-1 wants to merge 10 commits into
py-pdf:mainfrom
j-t-1:catalog
Open

MAINT: Deduplicate CatalogAttributes and CatalogDictionary#3868
j-t-1 wants to merge 10 commits into
py-pdf:mainfrom
j-t-1:catalog

Conversation

@j-t-1

@j-t-1 j-t-1 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Currently these classes represent the same thing. To simplify,
CatalogDictionary is deprecated with replacement CatalogAttributes.

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.85%. Comparing base (75e339c) to head (7d9dc5b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3868      +/-   ##
==========================================
- Coverage   97.85%   97.85%   -0.01%     
==========================================
  Files          57       57              
  Lines       10702    10701       -1     
  Branches     2001     2001              
==========================================
- Hits        10472    10471       -1     
  Misses        127      127              
  Partials      103      103              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@j-t-1

j-t-1 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Closes #3866.

@stefan6419846

Copy link
Copy Markdown
Collaborator

Why did you decide to use the name of the functionally less complex class?

@j-t-1

j-t-1 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

Why did you decide to use the name of the functionally less complex class?

In constants.py, CatalogDictionary is the only one ending in Dictionary in this variable:

PDF_KEYS = (
    AnnotationDictionaryAttributes,
    CatalogAttributes,
    CatalogDictionary,
    CcittFaxDecodeParameters,
    CheckboxRadioButtonAttributes,
    ColorSpaces,
    Core,
    DocumentInformationAttributes,
    EncryptionDictAttributes,
    FieldDictionaryAttributes,
    FileSpecificationDictionaryEntries,
    FilterTypeAbbreviations,
    FilterTypes,
    GoToActionArguments,
    GraphicsStateParameters,
    ImageAttributes,
    InteractiveFormDictEntries,
    LzwFilterParameters,
    PageAttributes,
    PageLayouts,
    PagesAttributes,
    Resources,
    StreamAttributes,
    TrailerKeys,
    TypArguments,
    TypFitArguments,
)

So went with that. Later noticed that CA is more readable than CD (although may be subjective).

Comment thread tests/test_constants.py Outdated
Comment thread tests/test_constants.py
match=r"^CatalogDictionary is deprecated and will be removed in pypdf 7\.0\.0\. "
r"Use CatalogAttributes instead\.$",
):
CatalogDictionary()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I hope nobody will be instantiating the constants classes. CatalogDictionary.NAMES will not trigger the deprecation at the moment and thus this needs further fixing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will do this, but unsure how.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We previously did this for single keys by implementing corresponding class properties, but this is most likely not feasible with this amount of keys.

I guess we could overwrite __getattr__ to issue warnings accordingly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@stefan6419846 with the deprecation CatalogDictionary does not exist, so cannot be added to:

CatalogDictionary = __create_old_class_instance

def _external_getattr(self, name):
    deprecate_with_replacement("CatalogDictionary", "CatalogAttributes", "7.0.0")
    return getattr(CatalogAttributes, name)

CatalogDictionary .__getattr__ = _external_getattr

What am I misunerstanding?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You have to keep it a class and overwrite all relevant attributes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

class CatalogDictionary:
    """§7.7.2 of the 1.7 and 2.0 references."""
    def __init__(self) -> None:
        deprecate_with_replacement("CatalogDictionary", "CatalogAttributes", "7.0.0")

    def __getattribute__(self, item: str) -> str:
        deprecate_with_replacement("CatalogDictionary", "CatalogAttributes", "7.0.0")
        return getattr(CatalogAttributes, item)
with pytest.warns(
        DeprecationWarning,
        match=r"^CatalogDictionary is deprecated and will be removed in pypdf 7\.0\.0\. "
              r"Use CatalogAttributes instead\.$",
):
    CatalogDictionary.TYPE

FAILED tests/test_constants.py::test_catalog_dictionary - Failed: DID NOT WARN. No warnings of type (<class 'DeprecationWarning'>,) were emitted.

What is the flaw in the class that is not giving a warning in the test?

__getattr__ and __getattribute__ gives the same error, and I will change back to __getattr__.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

__getattr__ might only trigger on class instances, not on direct attribute access. It seems this either needs a ton of classproperty definitions or some more magic.

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.

2 participants