Enforce strict Optional typing #723

FrNecas · 2022-07-14T12:26:44Z

Some of these fixes are not perfect, in a few places I added Optional where I feel it shouldn't really be. However, it's not really possible any other way without significant refactoring which would break the API (which I don't want to do). Sometimes a specific forge may return None so it was necessary to update the type of the abstract class.

I tried to not make any functional changes, most of these are just to align the typing with the actual implementation.

TODO:

regenerate some requre responses (the failing tests). I may need some help with this, I managed to do this for one of the failing tests but not for the repo creation yet.

Fixes #696

Related to #251

Merge before/after

softwarefactory-project-zuul · 2022-07-14T12:40:39Z

Build failed.

✔️ pre-commit SUCCESS in 1m 59s
❌ ogr-tests-rpm FAILURE in 6m 09s
❌ ogr-tests-pip-deps FAILURE in 6m 26s
✔️ ogr-reverse-dep-packit-tests SUCCESS in 13m 05s

jpopelka

Some of these fixes are not perfect

I see, it's been clear to me that the --no-strict-optional removal would mean some compromises.

TODO: regenerate some requre responses (the failing tests). I may need some help with this,

I'll try, but don't promise anything (I usually try to avoid this :-) )

mfocko · 2022-07-18T11:38:59Z

I'll have a look.

BTW it kinda bothers me that you need to regenerate data after typing-related changes :D

FrNecas · 2022-07-18T11:50:03Z

ogr/services/pagure/service.py

@@ -332,6 +332,8 @@ def project_create(
            parameters["description"] = repo
        if namespace:
            parameters["namespace"] = namespace
+        else:
+            namespace = self.user.get_username()


@mfocko in some cases, changing the code rather than the typing just seemed like the more correct solution. This is one of the places which causes the tests to fail. If namespace is not provided, it seems to assume the current authenticated user and fills it in automatically. However, PagureProject constructor requires namespace. If I hadn't added this extra call (which obviously creates the need for test data regeneration), the only solution would be to make namespace inside PagureProject Optional which does not seem right.

mfocko

yup, some of those Optionals are quite suspicious

mfocko · 2022-07-18T14:24:53Z

ogr/abstract.py

@@ -217,7 +217,7 @@ def body(self, new_body: str) -> None:
        self._body = new_body

    @property
-    def id(self) -> int:
+    def id(self) -> Optional[int]:


Does it make sense though?

I think I know what you meant in the description.

Yes, this is a bit of a dilemma. It doesn't make much sense semantically, however just based on our interface, this could happen. If I wanted to enforce an int here, the order of arguments in the constructor would have to be changed (along with some other changes of course)

ogr/abstract.py

ogr/parsing.py

mfocko · 2022-07-18T14:34:21Z

ogr/services/github/auth_providers/token.py

@@ -10,7 +10,9 @@


 class TokenAuthentication(GithubAuthentication):
-    def __init__(self, token: str, max_retries: Union[int, Retry] = 0, **_) -> None:
+    def __init__(
+        self, token: Optional[str], max_retries: Union[int, Retry] = 0, **_


oh no, memories of this magic are coming back /o\

ogr/services/github/flag.py

ogr/services/pagure/release.py

ogr/services/github/release.py

ogr/services/gitlab/issue.py

mfocko · 2022-07-18T14:42:30Z

ogr/services/gitlab/service.py

@@ -104,6 +104,8 @@ def project_create(
            except gitlab.GitlabGetError as ex:
                raise GitlabAPIException(f"Group {namespace} not found.") from ex
            data["namespace_id"] = group.id
+        else:
+            namespace = self.user.get_username()


this one is breaking tests, right?

I don't think so, only 4 pagure tests are failing right now. Perhaps getting the username was cached in the requre test data...

mfocko · 2022-07-18T14:43:21Z

ogr/services/pagure/pull_request.py

@@ -150,10 +150,11 @@ def create(
        }

        caller = project
-        if project.is_fork:
+        if project.is_fork and caller.parent:


tbf, this could break tests too, because you need to query both is_fork and parent, and on the background it may be 2 separate API calls, even though one is enough :/

Yes, this breaks something too IIRC.

mfocko · 2022-07-18T14:44:54Z

Oh, only 4 tests? I expected worse 😁

Signed-off-by: František Nečas <fnecas@redhat.com>

FrNecas · 2022-07-28T07:33:14Z

I've resolved some of the issues pointed out in the review, however I am still a bit unsure about the correct solution in some of the weird cases.

softwarefactory-project-zuul · 2022-07-28T07:47:24Z

Build failed.

✔️ pre-commit SUCCESS in 2m 41s
❌ ogr-tests-rpm FAILURE in 7m 07s
❌ ogr-tests-pip-deps FAILURE in 7m 05s
✔️ ogr-reverse-dep-packit-tests SUCCESS in 14m 09s

lachmanfrantisek

Wow, thanks so much for all the work!

lachmanfrantisek · 2022-07-28T09:14:13Z

ogr/abstract.py

-                - Pagure: namespace (e.g. `"rpms"`).
+                - Pagure: namespace (e.g. `"rpms"`). May be `None`, i.e. no namespace present.


What about using an empty string for this?

(I am looking at that from the GitLab perspecive where you can have "one_namespace", "two/namespaces", so why not "".)

I know, it's a functional change...;)

mfocko · 2022-07-28T09:18:34Z

I've resolved some of the issues pointed out in the review, however I am still a bit unsure about the correct solution in some of the weird cases.

I think one of the issues with types is that we were using some of the »abstract« classes in the test cases all over the codebase of packit(-service), which makes the typing more complicated.

IMO in ideal case we should have everything hidden behind a property, so that we can handle it from specific git-forge and maybe have a „dummy“ classes for tests. However that will involve breaking of a lot of rev-dep tests and seems, at least to me, time-consuming to fix everywhere :/

stale · 2022-10-30T13:38:40Z

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

stale · 2023-01-07T23:08:51Z

This issue has been marked as stale because it hasn't seen any
activity for the last 60 days.

Stale issues are closed after 14 days, unless the label is removed
by a maintainer or someone comments on it.

This is done in order to ensure that open issues are still relevant.

Thank you for your contribution! 🦄 🚀 🤖

(Note: issues labeled with pinned or EPIC are
never marked as stale.)

FrNecas changed the title ~~Frnecas typing~~ Enforce strict Optional typing Jul 14, 2022

jpopelka reviewed Jul 18, 2022

View reviewed changes

FrNecas commented Jul 18, 2022

View reviewed changes

mfocko reviewed Jul 18, 2022

View reviewed changes

František Nečas added 3 commits July 28, 2022 09:11

Pass Pagure tokens to test container

eeafa45

Signed-off-by: František Nečas <fnecas@redhat.com>

Enforce strict Optional typing

58572af

Signed-off-by: František Nečas <fnecas@redhat.com>

fixup! Enforce strict Optional typing

6d443e4

FrNecas force-pushed the frnecas-typing branch from df4174a to 6d443e4 Compare July 28, 2022 07:32

lachmanfrantisek reviewed Jul 28, 2022

View reviewed changes

lachmanfrantisek mentioned this pull request Dec 7, 2022

Strict mypy #251

Closed

stale bot added the stale Is the issue still valid? label Jan 7, 2023

jpopelka mentioned this pull request Sep 26, 2023

Ruff #806

Merged

mfocko added the 🌞 friday 🤟 Weekend is coming! label Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce strict Optional typing #723

Enforce strict Optional typing #723

FrNecas commented Jul 14, 2022

softwarefactory-project-zuul bot commented Jul 14, 2022

jpopelka left a comment

mfocko commented Jul 18, 2022

FrNecas Jul 18, 2022

mfocko left a comment

mfocko Jul 18, 2022

FrNecas Jul 18, 2022

mfocko Jul 18, 2022

mfocko Jul 18, 2022

FrNecas Jul 18, 2022

mfocko Jul 18, 2022

FrNecas Jul 18, 2022

mfocko commented Jul 18, 2022

FrNecas commented Jul 28, 2022

softwarefactory-project-zuul bot commented Jul 28, 2022

lachmanfrantisek left a comment

lachmanfrantisek Jul 28, 2022

mfocko commented Jul 28, 2022

stale bot commented Oct 30, 2022

stale bot commented Jan 7, 2023

		- Pagure: namespace (e.g. `"rpms"`).
		- Pagure: namespace (e.g. `"rpms"`). May be `None`, i.e. no namespace present.

Enforce strict Optional typing #723

Are you sure you want to change the base?

Enforce strict Optional typing #723

Conversation

FrNecas commented Jul 14, 2022

softwarefactory-project-zuul bot commented Jul 14, 2022

jpopelka left a comment

Choose a reason for hiding this comment

mfocko commented Jul 18, 2022

Choose a reason for hiding this comment

mfocko left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfocko commented Jul 18, 2022

FrNecas commented Jul 28, 2022

softwarefactory-project-zuul bot commented Jul 28, 2022

lachmanfrantisek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfocko commented Jul 28, 2022

stale bot commented Oct 30, 2022

stale bot commented Jan 7, 2023