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 the toml implementation from standard lib. #209

Closed
wants to merge 3 commits into from
Closed

Conversation

sallner
Copy link
Member

@sallner sallner commented Sep 22, 2023

Unfortunately, the standard lib only supplies read-only functionality. tomli-w is a small TOML writing library. I should be enough for our purpose, but it cannot write comments

Using tomlkit would allow fine more finely grained editing, but comes with a whole new data structure and needs a bigger refactoring.

The changes in this PR will change the formatting of the .meta.toml slightly, see the result for the Zope package

Diff of the .meta.toml in Zope
diff --git a/.meta.toml b/.meta.toml
index 9a080cba6..c352105f5 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 = "a3ddb354"
+commit-id = "77efeb4b"
 
 [python]
 with-pypy = false
@@ -26,23 +26,23 @@ known_zope = "AccessControl, Acquisition, App, DateTime, DocumentTemplate, Exten
 additional-config = [
     "# W503 line break before binary operator",
     "ignore = W503",
-    ]
+]
 
 [tox]
 testenv-deps = [
     "cffi",
-    ]
+]
 additional-envlist = [
     "pre-commit",
-    ]
+]
 testenv-commands-pre = [
     "{envbindir}/buildout -c {toxinidir}/buildout.cfg buildout:directory={envdir} buildout:develop={toxinidir} buildout:root-directory={toxinidir}",
-    ]
+]
 testenv-commands = [
     "# the `layer` argument below suppresses a `ZCatalog` performance test",
     "# which occasionally fails on GITHUB",
     "{envdir}/bin/alltests --layer '!Products.PluginIndexes' {posargs:-vc}",
-    ]
+]
 coverage-basepython = "python3.8"
 coverage-command = "coverage run {envdir}/bin/alltests {posargs:-vc}"
 testenv-additional = [
@@ -68,7 +68,7 @@ testenv-additional = [
     "commands =",
     "    autopep8 --verbose --in-place --recursive --aggressive --aggressive {toxinidir}/src setup.py",
     "    docformatter --in-place --recursive {toxinidir}/src setup.py",
-    ]
+]
 use-flake8 = true
 
 [manifest]
@@ -110,7 +110,7 @@ additional-rules = [
     "recursive-include src *.xml",
     "recursive-include src *.zcml",
     "recursive-include src *.zpt",
-    ]
+]
 
 [check-manifest]
 additional-ignores = [
@@ -123,13 +123,13 @@ additional-ignores = [
     "docs/_build/html/_sources/zopebook/includes/*",
     "docs/_build/html/_static/*",
     "docs/_build/html/_static/css/*",
-    ]
+]
 ignore-bad-ideas = [
     "src/Products/Five/tests/locales/de/LC_MESSAGES/fivetest.mo",
     "src/Products/Five/tests/locales/en/LC_MESSAGES/fivetest.mo",
-    ]
+]
 
 [git]
 ignore = [
     "docs/locale/*/LC_MESSAGES/*.mo",
-    ]
+]

Unfortunately, the standard lib only supplies read-only functionality. `tomli-w` is a small TOML writing library. I shoud be enough for our purpose, but it cannot write comments

Using `tomlkit` would allow fine more finely grained editing, but comes with a whole new data structure and needs a bigger refactoring.
@sallner sallner requested a review from icemac September 22, 2023 14:23
@sallner
Copy link
Member Author

sallner commented Sep 22, 2023

This solves #118.

Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

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

One small text change. I'll leave the actual review to Michael.

@@ -91,7 +91,8 @@ Usage
Preparation
+++++++++++

The script needs a ``venv`` with some packages installed::
The script needs a ``venv`` with at Python > 3.11 with some packages
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The script needs a ``venv`` with at Python > 3.11 with some packages
The script needs a ``venv`` with at least Python 3.11 and some dependencies

@dataflake
Copy link
Member

My gut feeling is that if we spend time on switching TOML libraries then it makes more sense to invest a little more and switch to tomlkit. This PR uses a mixture of tomli_w and tomllib where tomli_w is a bit lacking in capabilities. Using tomlkit would also remove the Python 3.11 limitation imposed by the use of tomllib.

@dataflake
Copy link
Member

See #215 for an alternative implementation that uses tomlkit

@icemac
Copy link
Member

icemac commented Mar 22, 2024

Superseded by #215.

Thank you for the initial implementation.

@icemac icemac closed this Mar 22, 2024
@icemac icemac deleted the use-tomllib branch March 22, 2024 17:14
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