Fix implementation checks on generic interfaces #109

ro-oliveira95 · 2023-11-21T13:49:59Z

Previously checking if a non-generic class implements a generic interface always used to fail, as the latter has a special method (__class_getitem__) that the former does't have, which led to false negatives.

Previously checking if a non-generic class implements a generic interface always used to fail, as the latter has a special method (`__class_getitem__`) that the former does't have, which led to false negatives.

prusse-martin · 2023-11-21T17:54:03Z

src/oop_ext/interface/_tests/test_interface.py

+    class IFoo(Interface, Generic[T], TypeCheckingSupport):
+        # Class instance defined here as a workaround for this class to work in Python 3.6.
+        __abstractmethods__: FrozenSet = frozenset()


@nicoddemus
Do you think we could drop python 3.6 support in oop-ext?

Definitely, IMO we can do this for all of our OS projects, supporting >=3.7 or even >=3.10 only, if the former is too much work.

src/oop_ext/interface/_interface.py

prusse-martin · 2023-11-21T18:01:18Z

@nicoddemus

The sonar cloud token is a organization wide configuration?
Here it is failing to access the token. Any ideas?

prusse-martin · 2023-11-21T18:16:55Z

There was an early attempt at this in the past:
#97

nicoddemus · 2023-12-05T10:41:34Z

CHANGELOG.rst

@@ -2,6 +2,28 @@
 ------------------

 * Added support for Python 3.10.
+* #109: Non-generic implementers of generic interfaces are now correctly checked.


Great changelog entry. 👍

nicoddemus · 2023-12-05T10:42:34Z

src/oop_ext/interface/_tests/test_interface.py

+    class IFoo(Interface, Generic[T], TypeCheckingSupport):
+        # Class instance defined here as a workaround for this class to work in Python 3.6.
+        __abstractmethods__: FrozenSet = frozenset()


Definitely, IMO we can do this for all of our OS projects, supporting >=3.7 or even >=3.10 only, if the former is too much work.

nicoddemus · 2023-12-05T10:46:22Z

CHANGELOG.rst

@@ -2,6 +2,28 @@
 ------------------

 * Added support for Python 3.10.
+* #109: Non-generic implementers of generic interfaces are now correctly checked.


Suggested change

* #109: Non-generic implementers of generic interfaces are now correctly checked.

* `#109 <https://github.com/ESSS/oop-ext/pull/109>`_: Non-generic implementers of generic interfaces are now correctly checked.

nicoddemus · 2023-12-08T12:45:43Z

src/oop_ext/interface/_tests/test_interface.py

+        def GetOutput(self) -> T:  # type:ignore[empty-body]
+            ...


To solve this new warning we are decorating interface methods with @abstractmethod, so mypy won't complain. Also let's make the example more comprehensive by also having an input parameter:

Suggested change

def GetOutput(self) -> T: # type:ignore[empty-body]

...

@abstractmethod

def GetOutput(self, v: T) -> T:

...

nicoddemus · 2023-12-08T12:46:14Z

src/oop_ext/interface/_tests/test_interface.py

+        # Class instance defined here as a workaround for this class to work in Python 3.6.
+        __abstractmethods__: FrozenSet = frozenset()


Let's drop Python 3.6 first.

Suggested change

# Class instance defined here as a workaround for this class to work in Python 3.6.

__abstractmethods__: FrozenSet = frozenset()

To drop Python 3.6:

Review CI: https://github.com/ESSS/oop-ext/blob/master/.github/workflows/main.yml

Update setup.py:

oop-ext/setup.py

Line 35 in 9b6f944

"Programming Language :: Python :: 3.6",

oop-ext/setup.py

Line 47 in 9b6f944

python_requires=">=3.6",

Update tox.ini:

oop-ext/tox.ini

Line 2 in 9b6f944

envlist = py36, py37, py38, py39, docs

Update CHANGELOG: https://github.com/ESSS/oop-ext/blob/master/CHANGELOG.rst

Update environment.devenv.yml:

oop-ext/environment.devenv.yml

Line 7 in 9b6f944

- python>=3.6

prusse-martin · 2023-12-08T17:37:41Z

Definitely, IMO we can do this for all of our OS projects, supporting >=3.7 or even >=3.10 only, if the former is too much work.

I think 3.7 has reached EOL.

Yep. It did.
https://devguide.python.org/versions/

nicoddemus · 2023-12-08T17:38:36Z

I think 3.7 has reached EOL.

You are right, let's drop that one too.

prusse-martin · 2023-12-08T17:42:53Z

CHANGELOG.rst

+      class IFoo(Interface, typing.Generic[T]):
+          def Bar(self) -> T:
+              ...
+
+
+      @ImplementsInterface(IFoo, no_check=True)
+      class Foo:
+          @Implements(IFoo.Bar)
+          def Bar(self) -> str:
+              return "baz"
+
+
+      AssertImplements(Foo, IFoo)  # Ok now.


And without the no_check? Does the interface infra complain?

To appease mypy do we still have to declare TypeCheckingSupport?

And without the no_check? Does the interface infra complain?

Yes, it does complain.

To appease mypy do we still have to declare TypeCheckingSupport?

Yeah we still have to.

And without the no_check? Does the interface infra complain?

Yes, it does complain.

Why? How?

prusse-martin · 2023-12-08T17:45:54Z

src/oop_ext/interface/_tests/test_interface.py

+    AssertImplements(GenericFoo, IFoo)
+    AssertImplements(GenericFoo[str](output="foo"), IFoo)
+
+    # This only works if we skip the verification of `__class_getitem__` method.
+    AssertImplements(NonGenericFoo, IFoo)


Just a question.

What about AssertImplements(GenericFoo[str], IFoo)?

This makes it raise an AssertionError with the following error:

E AssertionError: Method 'GetOutput' is missing in class '_GenericAlias' (required by interface 'IFoo')
E type object '_GenericAlias' has no attribute 'GetOutput'

Don't we want to handle that?

AssertImplements(GenericFoo[str], IFoo[str]) also fail?

ro-oliveira95 marked this pull request as draft November 21, 2023 13:56

Fix implementation checks on generic interfaces

9ee227d

Previously checking if a non-generic class implements a generic interface always used to fail, as the latter has a special method (`__class_getitem__`) that the former does't have, which led to false negatives.

ro-oliveira95 force-pushed the fix-check-on-generic-interfaces branch from 32edb03 to 9ee227d Compare November 21, 2023 17:33

ro-oliveira95 changed the title ~~Fix lack of support on checking generic interfaces~~ Fix implementation checks on generic interfaces Nov 21, 2023

ro-oliveira95 marked this pull request as ready for review November 21, 2023 17:39

prusse-martin reviewed Nov 21, 2023

View reviewed changes

src/oop_ext/interface/_interface.py Show resolved Hide resolved

nicoddemus approved these changes Dec 5, 2023

View reviewed changes

nicoddemus reviewed Dec 5, 2023

View reviewed changes

nicoddemus requested changes Dec 8, 2023

View reviewed changes

prusse-martin approved these changes Dec 8, 2023

View reviewed changes

ro-oliveira95 marked this pull request as draft May 21, 2024 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix implementation checks on generic interfaces #109

Fix implementation checks on generic interfaces #109

ro-oliveira95 commented Nov 21, 2023 •

edited

Loading

prusse-martin Nov 21, 2023

nicoddemus Dec 5, 2023 •

edited

Loading

prusse-martin commented Nov 21, 2023

prusse-martin commented Nov 21, 2023

nicoddemus Dec 5, 2023

nicoddemus Dec 5, 2023 •

edited

Loading

nicoddemus Dec 5, 2023

nicoddemus Dec 8, 2023

nicoddemus Dec 8, 2023

nicoddemus Dec 8, 2023

prusse-martin commented Dec 8, 2023 •

edited

Loading

nicoddemus commented Dec 8, 2023

prusse-martin Dec 8, 2023

ro-oliveira95 Dec 11, 2023

prusse-martin Dec 12, 2023

prusse-martin Dec 8, 2023

ro-oliveira95 Dec 11, 2023

prusse-martin Dec 12, 2023

	* #109: Non-generic implementers of generic interfaces are now correctly checked.
	* `#109 <https://github.com/ESSS/oop-ext/pull/109>`_: Non-generic implementers of generic interfaces are now correctly checked.

		# Class instance defined here as a workaround for this class to work in Python 3.6.
		__abstractmethods__: FrozenSet = frozenset()

Fix implementation checks on generic interfaces #109

Are you sure you want to change the base?

Fix implementation checks on generic interfaces #109

Conversation

ro-oliveira95 commented Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

nicoddemus Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

prusse-martin commented Nov 21, 2023

prusse-martin commented Nov 21, 2023

Choose a reason for hiding this comment

nicoddemus Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusse-martin commented Dec 8, 2023 • edited Loading

nicoddemus commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro-oliveira95 commented Nov 21, 2023 •

edited

Loading

nicoddemus Dec 5, 2023 •

edited

Loading

nicoddemus Dec 5, 2023 •

edited

Loading

prusse-martin commented Dec 8, 2023 •

edited

Loading