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

crs.py: remove @abstractmethod decorator #1018

Merged
merged 1 commit into from Jan 26, 2022
Merged

Conversation

hemberger
Copy link
Contributor

According to the documentation for the @abstractmethod decorator:

Using this decorator requires that the class's metaclass is
ABCMeta or is derived from it.

(source: https://docs.python.org/3/library/abc.html#abc.abstractmethod)

Since CustomConstructorCRS and CRS are not defined as abstract
classes, using the @abstractmethod decorator does not actually do
anything and should be removed.

  • Tests added
  • Fully documented, including history.rst for all changes and api/*.rst for new API

According to the documentation for the `@abstractmethod` decorator:

> Using this decorator requires that the class's metaclass is
> ABCMeta or is derived from it.

(source: https://docs.python.org/3/library/abc.html#abc.abstractmethod)

Since `CustomConstructorCRS` and `CRS` are not defined as abstract
classes, using the `@abstractmethod` decorator does not actually do
anything and should be removed.
@hemberger
Copy link
Contributor Author

A couple notes about this PR:

  1. Even though the python interpreter ignores @abstractmethod here, it can still cause a problem for linters. For example:

    import abc 
    import pyproj.crs
    
    class Foo(pyproj.crs.CustomConstructorCRS, metaclass=abc.ABCMeta):
        pass
    
    Foo()

    This runs properly, but pylint will report the following error:

    $ pylint --errors-only test.py
    ************* Module test
    test.py:6:0: E0110: Abstract class 'Foo' with abstract methods instantiated (abstract-class-instantiated)
    

    While this is a false positive that might be fixable in pylint, it is still a symptom of @abstractmethod being used improperly.

  2. An alternative solution would be to make CustomConstructorCRS an abstract class, but this seems too heavy-handed since it would break backwards compatibility for any class that derived from CustomConstructorCRS, but which has not implemented the _expected_types method (which has an entirely reasonable implementation in the CustomConstructorCRS class). This is the case with, for example, the CRS wrapper class in the cartopy module.

Please let me know if any changes need to be made to this PR to make it suitable for merging. Thank you for your time!

@snowman2 snowman2 added this to In progress in 3.3.1 Release via automation Jan 26, 2022
@snowman2
Copy link
Member

Thanks @hemberger 👍

@snowman2 snowman2 merged commit ce9de36 into pyproj4:main Jan 26, 2022
3.3.1 Release automation moved this from In progress to Done Jan 26, 2022
@hemberger hemberger deleted the fix-abc branch February 2, 2022 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants