search

claird / PyPDF4

A utility to read and write PDFs with Python
obsolete-https://pythonhosted.org/PyPDF2/
Other
328 stars 61 forks source link

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

Closed acsor closed 5 years ago

acsor commented 5 years ago

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 commented 5 years ago

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

claird commented 5 years ago

"isEncrypted" is better style than "getIsEncrypted" in any realistic sense.

Cameron Laird, vice president We make computers work for people.

On Fri, Oct 5, 2018 at 5:13 PM Oscar notifications@github.com wrote:

I introduced this modification in Remove redundant @Property accessors from PdfFileReader. https://github.com/claird/PyPDF4/commit/f020613e99b05ab982acca04519428fd645efbf7. Before I close this issue we can eventually discuss the change.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/claird/PyPDF4/issues/19#issuecomment-427512769, or mute the thread https://github.com/notifications/unsubscribe-auth/AAbN9AGkOQ3pliFWehw883YkIVmTp2iyks5uh9mDgaJpZM4W0Mxr .

acsor commented 5 years ago

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 :'-).