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 tomlkit for TOML read/write operations #215

Merged
merged 5 commits into from
Mar 22, 2024
Merged

Conversation

dataflake
Copy link
Member

This PR is an alternative to #209. It does two things:

  • replace toml use with tomlkit
  • update dependencies in requirements.txt to the latest versions that still support Python 3.7

Running this code against current Zope yields the following diff in .meta.toml:

diff --git a/.meta.toml b/.meta.toml
index 82f0c085f..07cac2b24 100644
--- a/.meta.toml
+++ b/.meta.toml
@@ -2,7 +2,7 @@
 # https://github.com/zopefoundation/meta/tree/master/config/zope-product
 [meta]
 template = "zope-product"
-commit-id = "571024f0"
+commit-id = "babc889c"

 [python]
 with-pypy = false
@@ -11,6 +11,7 @@ with-sphinx-doctests = false
 with-windows = true
 with-macos = true
 with-future-python = true
+with-appveyor = false

 [coverage]
 fail-under = 80

The different is most likely due to the change in the _set_python_config_value method where I always use the default False because some values were None at that point, which tomlkit didn't like.

@dataflake dataflake requested review from icemac and sallner October 7, 2023 09:31
@dataflake dataflake self-assigned this Oct 7, 2023
@dataflake
Copy link
Member Author

Instead of changing the behavior of _set_python_config_value I have decided to fix the real reason: Several script arguments that want to store true or false had an inconsistent default of None. I have corrected those. The outcome is still the same as the diff shown above.

@sallner
Copy link
Member

sallner commented Oct 7, 2023

We can also do it this way, but it actually does not use the capabilities of tomlkit to add and preserve comments now. It does the same as before, so I am fine from this perspective.

Copy link
Member

@sallner sallner left a comment

Choose a reason for hiding this comment

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

It looks okay to me, although we might end up with all default values in the meta.toml now.

@dataflake
Copy link
Member Author

It looks okay to me, although we might end up with all default values in the meta.toml now.

I was thinking about that and to be honest I don't think that's a bad thing. It's very explicit.

Copy link
Member

@icemac icemac left a comment

Choose a reason for hiding this comment

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

Finally I got to this PR.

@icemac icemac merged commit f631b48 into master Mar 22, 2024
3 checks passed
@icemac icemac deleted the dataflake/tomlkit branch March 22, 2024 17:12
@icemac
Copy link
Member

icemac commented Mar 22, 2024

Thank you for this PR. 😃

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