py-pdf / pypdf

A pure-python PDF library capable of splitting, merging, cropping, and transforming the pages of PDF files
https://pypdf.readthedocs.io/en/latest/
Other
7.73k stars 1.36k forks source link

DOC: Improve slightly docstrings and documentation #2689

Closed j-t-1 closed 1 month ago

j-t-1 commented 1 month ago

In classes Ellipse and Rectangle, can I change interiour_color to be interior_color?

stefan6419846 commented 1 month ago

Renaming a parameter is only possible in accordance with our deprecation process.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.12%. Comparing base (7710a3c) to head (3762ede).

:exclamation: Current head 3762ede differs from pull request most recent head 97756d5

Please upload reports for the commit 97756d5 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2689 +/- ## ======================================= Coverage 95.12% 95.12% ======================================= Files 51 51 Lines 8521 8521 Branches 1700 1700 ======================================= Hits 8106 8106 Misses 261 261 Partials 154 154 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

j-t-1 commented 1 month ago

Renaming a parameter is only possible in accordance with our deprecation process.

Would this be how it is done?

class Ellipse(MarkupAnnotation):
    def __init__(
        self,
        rect: Union[RectangleObject, Tuple[float, float, float, float]],
        *,
        deprecate_with_replacement("interiour_color", "interior_color", "5.0.0")
        interiour_color: Optional[str] = None,
        **kwargs: Any,
    ):

Worth it?

stefan6419846 commented 1 month ago

Rather something like this:

class Ellipse(MarkupAnnotation):
    def __init__(
        self,
        rect: Union[RectangleObject, Tuple[float, float, float, float]],
        *,
        interior_color: Optional[str] = None,
        **kwargs: Any,
    ):
        if kwargs.get("interiour_color"):
            interior_color = kwargs["interiour_color"]
            deprecate_with_replacement("interiour_color", "interior_color", "6.0.0")
        ...
stefan6419846 commented 1 month ago

It seems like CI does not really like this, thus we might need to explicitly specify both parameters instead of using kwargs.get, although I am not sure why.

j-t-1 commented 1 month ago

Is it to do with the order of the * argument? Probably I need to also change in rect=(50, 550, 200, 650), interiour_color="ff0000" in tests/test_generic.py.

stefan6419846 commented 1 month ago

The * usually divides positional and keyword only arguments. In general, the deprecation cannot be done unless we are sure that both the old and the new name work correctly.

j-t-1 commented 1 month ago

So * is essentially a non-functional delimiter between positional and keyword only arguments?

Is the accepted answer here a solution for us? https://stackoverflow.com/questions/49802412/how-to-implement-deprecation-in-python-with-argument-alias

class Ellipse(MarkupAnnotation):
    def __init__(
        self,
        rect: Union[RectangleObject, Tuple[float, float, float, float]],
        *,
        interior_color: Optional[str] = None,
        interiour_color: Optional[str] = None,
        **kwargs: Any,
    ):
stefan6419846 commented 1 month ago

Regarding the separator behavior: Basically yes, see corresponding PEP 3102 as well.

The solution you propose basically is the same I have mentioned in https://github.com/py-pdf/pypdf/pull/2689#issuecomment-2134759931.

j-t-1 commented 1 month ago

I agree. If the first commit was okay, you could merge just that, then I will do a fresh PR concentrating on this one variable name deprecation?

stefan6419846 commented 1 month ago

Given the title, doing a separate PR for the deprecation sounds like the better idea. Then you can test your changes locally as well without any pressure.

j-t-1 commented 1 month ago

Will do; does this mean you will merge the first commit?

stefan6419846 commented 1 month ago

If the remaining commits are reverted and only the original changes are present and CI passes, I am open to merging the changes.