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

Use tomllib from the standard library on Python 3.11 #274

Merged
merged 1 commit into from Oct 17, 2022

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented Oct 6, 2022

The code would be slightly easier if we fell back to tomli, but I kept using toml as this code also targets EL 8.

See https://fedoraproject.org/wiki/Changes/DeprecatePythonToml

@abompard abompard changed the base branch from stable to develop October 14, 2022 15:01
@hroncok
Copy link
Contributor Author

hroncok commented Oct 14, 2022

FTR tomli is being added to epel8, so this could be made a bit simpler.

@hroncok
Copy link
Contributor Author

hroncok commented Oct 14, 2022

@abompard
Copy link
Member

OK, do you want to rework it Miro?

@hroncok
Copy link
Contributor Author

hroncok commented Oct 14, 2022

Done.

@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2022

Codecov Report

Base: 88.53% // Head: 88.76% // Increases project coverage by +0.23% 🎉

Coverage data is based on head (5bfed20) compared to base (5195026).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 5bfed20 differs from pull request most recent head b9eb8b1. Consider uploading reports for the commit b9eb8b1 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #274      +/-   ##
===========================================
+ Coverage    88.53%   88.76%   +0.23%     
===========================================
  Files           14       13       -1     
  Lines         1186     1193       +7     
  Branches       173      174       +1     
===========================================
+ Hits          1050     1059       +9     
+ Misses         123      121       -2     
  Partials        13       13              
Flag Coverage Δ
unittests 88.68% <100.00%> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
fedora_messaging/config.py 100.00% <100.00%> (ø)
fedora_messaging/__init__.py
fedora_messaging/message.py 94.91% <0.00%> (+0.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hroncok
Copy link
Contributor Author

hroncok commented Oct 14, 2022

The mocks got unhappy, trying to fix that.

@hroncok

This comment was marked as resolved.

@hroncok hroncok marked this pull request as draft October 14, 2022 17:00
@hroncok
Copy link
Contributor Author

hroncok commented Oct 14, 2022

This turned out to be more tedious than I anticipated. The tests are very unforgiving to slight implementation changes :(

Anyway, I think I cracked it and

$ tox -e=py3{6,7,8,9,10,11}-unittest

Passes.

Let me know if I shall squash or if you'd rather squash when merging. I'd like you to see my train of thought when reviewing it, so not squashing yet.

@hroncok hroncok marked this pull request as ready for review October 14, 2022 17:13
@abompard
Copy link
Member

Let me know if I shall squash or if you'd rather squash when merging. I'd like you to see my train of thought when reviewing it, so not squashing yet.

Yeah, got it, thanks. Please squash. Sorry it ended up more painful that it should have been.

Fallback to tomli otherwise.

See https://fedoraproject.org/wiki/Changes/DeprecatePythonToml

Signed-off-by: Miro Hrončok <miro@hroncok.cz>
@hroncok
Copy link
Contributor Author

hroncok commented Oct 15, 2022

Squashed, signed off.

@abompard abompard merged commit 038b9a1 into fedora-infra:develop Oct 17, 2022
@hroncok hroncok deleted the tomllib branch October 17, 2022 10:21
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.

None yet

3 participants