ogr icon indicating copy to clipboard operation
ogr copied to clipboard

Enforce strict Optional typing

Open FrNecas opened this issue 2 years ago • 8 comments

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

FrNecas avatar Jul 14 '22 12:07 FrNecas

Build failed.

:heavy_check_mark: pre-commit SUCCESS in 1m 59s :x: ogr-tests-rpm FAILURE in 6m 09s :x: ogr-tests-pip-deps FAILURE in 6m 26s :heavy_check_mark: ogr-reverse-dep-packit-tests SUCCESS in 13m 05s

I'll have a look.

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

mfocko avatar Jul 18 '22 11:07 mfocko

Oh, only 4 tests? I expected worse :grin:

mfocko avatar Jul 18 '22 14:07 mfocko

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.

FrNecas avatar Jul 28 '22 07:07 FrNecas

Build failed.

:heavy_check_mark: pre-commit SUCCESS in 2m 41s :x: ogr-tests-rpm FAILURE in 7m 07s :x: ogr-tests-pip-deps FAILURE in 7m 05s :heavy_check_mark: ogr-reverse-dep-packit-tests SUCCESS in 14m 09s

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 :/

mfocko avatar Jul 28 '22 09:07 mfocko

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! :unicorn: :rocket: :robot:

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

stale[bot] avatar Oct 30 '22 13:10 stale[bot]

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! :unicorn: :rocket: :robot:

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

stale[bot] avatar Jan 07 '23 23:01 stale[bot]