-
Notifications
You must be signed in to change notification settings - Fork 466
Change python-toml dependency to tomli #3860
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution!
This is a breaking change, but I think it's OK. @Kwpolska what do you think; also do we have a policy on such breaking changes?
Co-authored-by: Felix Fontein <felix@fontein.de>
"tomli;python_version<'3.11'", | ||
"tomli-w", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please specify a minimum version. The one available in Debian stable or Ubuntu LTS will probably be the best option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fairly arbitrary, I would prefer to leave it to upstream to decide on a minimum. I generally do not add bounds until I find the need for one at which point I document the need for it. It is too easy for the bounds to become out-of-sync with the actual usage in the code itself (although there are CI tricks to test the lower-bounds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We consider minimums necessary to avoid issues when people come with ancient versions and expect things to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only versions packaged for non-EOL versions of Ubuntu (22.04+) and Debian (Bookworm+) is 1.0.0. So tomli-w>=1.0.0
should definitely be fine.
Also we're using tomli-w in exactly one line, and the usage there has apparently been supported by tomli-w since its first release (0.1.0) according to the changelog (https://github.com/hukkin/tomli-w/blob/master/CHANGELOG.md).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for tomli
for that matter. They were made to be compatible up to only a few interface changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about setting both >=1.0.0
then?
"tomli;python_version<'3.11'", | ||
"tomli-w", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"tomli;python_version<'3.11'", | |
"tomli-w", | |
"tomli>=1.0;python_version<'3.11'", | |
"tomli-w>=1.0", |
Pull Request Checklist
Description
This is a quick PR to move the dependency from
toml
totomllib
/tomli
+tomli-w
. The old dependency has not been maintained for a while and downstream packagers have deprecated it and it will soon be removed.Related fedora change: https://fedoraproject.org/wiki/Changes/DeprecatePythonToml (this is quite old and most of the dependencies have been resolved, we are dealing now with the remaining leftovers)