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

Collapse getProperty() and property @properties from PdfFileReader into one #19

Closed
acsor opened this issue Sep 21, 2018 · 3 comments
Closed

Comments

@acsor
Copy link
Collaborator

acsor commented Sep 21, 2018

PdfFileReader has about 9 cases of the following pattern. Take as a concrete example isEncrypted:

    def getIsEncrypted(self):
        return "/Encrypt" in self._trailer

    isEncrypted = property(getIsEncrypted)
    """
    Read-only boolean property showing whether this PDF file is encrypted.
    Note that this property, if True, will remain True even after the
    :meth:`decrypt()<PdfFileReader.decrypt>` method is called.
    """

I do not see any real utility in this code style and it actually causes more verbosity. I invite to change these many cases were getProperty() == property to something like (take isEncrypted as an example again):

"""
Read-only boolean property showing whether this PDF file is encrypted.
Note that this property, if True, will remain True even after the
:meth:`decrypt()<PdfFileReader.decrypt>` method is called.
"""
@property
def isEncrypted(self):
        return "/Encrypt" in self._trailer

Please note that isEncrypted's docstring seems misleading. Indeed, I know of no default caching for property decorators, as attested here.

@acsor
Copy link
Collaborator Author

acsor commented Oct 5, 2018

I introduced this modification in Remove redundant @Property accessors from PdfFileReader. . Before I close this issue we can eventually discuss the change.

@claird
Copy link
Owner

claird commented Oct 5, 2018 via email

@acsor
Copy link
Collaborator Author

acsor commented Oct 5, 2018

Yeah, definitely. Also, the past getIsEncrypted() code was so minimal that I was left wondering why no one did like so in the first place :'-).

@acsor acsor closed this as completed Nov 3, 2018
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

No branches or pull requests

2 participants