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/tomli/tomli-w instead of unmaintained toml [WIP] #596

Merged
merged 1 commit into from Oct 14, 2022

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Oct 12, 2022

Replace the toml dependency that is unmaintained (last release in 2020) and does not implement TOML 1.0 with more modern libraries. Use the built-in tomllib module to load .toml files on Python 3.11, and its drop-in replacement tomli package on older versions of Python. For writing .toml files, use the tomli-w package.

@mgorny
Copy link
Contributor Author

mgorny commented Oct 12, 2022

There's one problem with this: right now it fails because it attempts to serialize None that has no valid representation in TOML. Apparently the buggy toml module either ignored keys with None as value or serialized it as string "None" (sic!).

Copy link
Collaborator

@beliaev-maksim beliaev-maksim left a comment

Choose a reason for hiding this comment

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

we will add support for 3.11 once it is released
please remove it so far

does tomli include type annotations as inlines?

@mgorny
Copy link
Contributor Author

mgorny commented Oct 12, 2022

we will add support for 3.11 once it is released please remove it so far

That sounds quite pointless, given it's at rc2 now and the API is frozen already but sure.

does tomli include type annotations as inlines?

Yes.

@mgorny mgorny force-pushed the tomli branch 2 times, most recently from 477f25e to dd6cfc2 Compare October 12, 2022 12:32
responses/tests/test_recorder.py Outdated Show resolved Hide resolved
responses/__init__.py Outdated Show resolved Hide resolved
@beliaev-maksim
Copy link
Collaborator

makes sense to switch. Thanks for the PR

at the time of writing the feature I chose toml because of the wider usage by the community. However, if it is unmaintained there is no reason not to go for tomli

will also resolve #595

@mgorny
Copy link
Contributor Author

mgorny commented Oct 12, 2022

at the time of writing the feature I chose toml because of the wider usage by the community.

I'm surprised at that but I suppose there's still work to be done there. I think most of the major projects (black, flit, mypy, pytest, setuptools, tox to list just a few) have switched to tomli already. At least a quick glance it seems that tomli has more revdeps in Gentoo than toml.

@beliaev-maksim
Copy link
Collaborator

@mgorny
please run tox until all are passed and ensure 100% code coverage

https://github.com/getsentry/responses#tests-and-code-quality-validation

@mgorny
Copy link
Contributor Author

mgorny commented Oct 12, 2022

See #596 (comment). The problem is that the input is broken right now, and I'm looking for guidance what fix would you prefer for it.

@beliaev-maksim
Copy link
Collaborator

Apparently the buggy toml module either ignored keys with None

it is not buggy, but done on purpose
I just went over discussions on toml-lang/toml#921
also, read threads in tomli-w and toml

according to toml-lang the value with null should not be present in the file, thus, it should be filtered out in languages where it is present eg python. So, we need to filter all nulls out.

def remove_nones(d):
    if isinstance(d, dict):
        return {k: remove_nones(v) for k, v in d.items() if v is not None}
    return d

and then dump the result

Replace the `toml` dependency that is unmaintained (last release
in 2020) and does not implement TOML 1.0 with more modern `tomli`
and `tomli-w` packages.

Co-authored-by: Maksim Beliaev <beliaev.m.s@gmail.com>
@mgorny
Copy link
Contributor Author

mgorny commented Oct 12, 2022

Thanks for the snippet. Done that now and the tests seem to pass.

@codecov
Copy link

codecov bot commented Oct 14, 2022

Codecov Report

Base: 100.00% // Head: 99.92% // Decreases project coverage by -0.07% ⚠️

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

Additional details and impacted files
@@             Coverage Diff             @@
##            master     #596      +/-   ##
===========================================
- Coverage   100.00%   99.92%   -0.08%     
===========================================
  Files            9        9              
  Lines         2775     2788      +13     
===========================================
+ Hits          2775     2786      +11     
- Misses           0        2       +2     
Impacted Files Coverage Δ
responses/__init__.py 99.60% <100.00%> (-0.40%) ⬇️
responses/_recorder.py 100.00% <100.00%> (ø)
responses/tests/test_recorder.py 100.00% <100.00%> (ø)

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.

@markstory markstory merged commit 972b461 into getsentry:master Oct 14, 2022
@markstory
Copy link
Member

Thank you 🎉

@mgorny
Copy link
Contributor Author

mgorny commented Oct 15, 2022

Thanks!

@mgorny mgorny deleted the tomli branch October 15, 2022 06:32
@mgorny
Copy link
Contributor Author

mgorny commented Oct 29, 2022

we will add support for 3.11 once it is released please remove it so far

Python 3.11.0 has been released on Oct 24th.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants