Skip to content

Conversation

@jku
Copy link
Member

@jku jku commented Nov 29, 2024

This PR achieves two things:

  • Makes us compatible with Python 3.8 again (a non-supported release by now but I figured it's nice to not be the first one to break compatibility...)
  • Starts using several nice annotation features from Python 3.10, e.g.:
    • X | Y instead of typing.Union[X, Y],
    • X | None instead of typing.Optional[X],
    • No quotes around class names in factory method return values

Almost all changes come from ruff autofixes: there should be no functional changes here. There's a small revert included to avoid the one thing that was incompatible with python 3.8.

This was tested by setting "--python-version 3.8" in our mypy invocation... Ideally we should be linting using the lowest supported python version but I did not include that change in this PR

jku added 2 commits November 29, 2024 11:53
These are no longer part of the ruleset

Signed-off-by: Jussi Kukkonen <[email protected]>
This allows using some more nice annotations from 3.10
while still being compatible with even Python 3.8.

These are all annotation changes, should not modify any functionality.

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku requested a review from a team as a code owner November 29, 2024 10:51
This reverts commit eb6d82f.

The change itself was fine but since the code is otherwise compatible
with python 3.8, let's revert this to be compatible for one more
release.

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the use-future-annotations branch from aca4a7e to 687d455 Compare November 29, 2024 10:51
jku added 2 commits November 29, 2024 13:19
These are assertions that should happen in production:
something is wrong in an unrecoverable way.

This is not an API change since no-one should be catching these.
Making these AssertionErrors makes them skippable in coverage.

Signed-off-by: Jussi Kukkonen <[email protected]>
This makes the results more useful

Signed-off-by: Jussi Kukkonen <[email protected]>
@jku jku force-pushed the use-future-annotations branch from c08cafb to d89c8e6 Compare November 29, 2024 11:29
@coveralls
Copy link

coveralls commented Nov 29, 2024

Pull Request Test Coverage Report for Build 12086553417

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 53 of 69 (76.81%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.6%) to 96.012%

Changes Missing Coverage Covered Lines Changed/Added Lines %
examples/manual_repo/hashed_bin_delegation.py 3 4 75.0%
tuf/api/_payload.py 23 24 95.83%
tuf/api/metadata.py 5 6 83.33%
tuf/ngclient/_internal/requests_fetcher.py 4 5 80.0%
tuf/ngclient/updater.py 5 6 83.33%
tuf/ngclient/_internal/trusted_metadata_set.py 4 6 66.67%
tuf/repository/_repository.py 4 13 30.77%
Totals Coverage Status
Change from base Build 12082518232: -0.6%
Covered Lines: 1582
Relevant Lines: 1627

💛 - Coveralls

@jku
Copy link
Member Author

jku commented Nov 29, 2024

Ok, there is now a tiny runtime change as well: Repository module changes an exception type to allow for better coverage management. This is not an API change since the exception is not meant to be caught: it really is an assertion purely for defensive programming purposes.

We're still compatible with 3.8: let's not force 3.9 yet.

Signed-off-by: Jussi Kukkonen <[email protected]>
@kairoaraujo
Copy link
Contributor

Thank you @jku
I'm blocking some time to review it.

Copy link
Contributor

@kairoaraujo kairoaraujo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jku jku merged commit d805a81 into theupdateframework:develop Dec 6, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants