Toml Configuration #973
Closed
Toml Configuration #973
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ryneeverett
force-pushed
the
toml
branch
6 times, most recently
from
dda23a7
to
6ad3c09
Compare
ryneeverett
force-pushed
the
toml
branch
6 times, most recently
from
066493e
to
8b24019
Compare
This was referenced Feb 26, 2023
While less readable and more verbose, this is generalizable to future configuration language parsers which likewise implement a dictionary interface.
Adding a new configuration file format seems like an appropriate time to drop the "prefix" requirement. While we want to maintain backwards compatibility, maintaining "prefix awareness" within the validation which is only relevant during INI validation would require even more intricate metaprogramming and I don't think it's worth it. It's questionable whether the existing amount of error message enhancement is worth the future maintenance burden, so I think deleting some of it is worth the cost of somewhat degrading the error messages. They're still better than they were in the last release. As an additional side-effect though, bugwarrior will no longer care if prefixes are correct or if they're even present for INI files. If I recall correctly, in previous releases these were be silently ignored, but now they'll "just work". I'm not sure it really matters... Along with stripping prefixes, we're also converting the ConfigParser instance to a dictionary before passing it into validation, which will be necessary so that future configuration parsers can also pass this universal format. This means that almost everywhere in the test suite we were building ConfigParser's can now just use a dictionary. Since the schema is now unaware of configparser features we remove the requirement of a DEFAULT section, which is included in any ConfigParser instance but will not be included in the dictionaries provided by other configuration language parsers.
This removes the last of the periods from our option names. This will
not work with toml because periods are interpreted as nested options in
toml. That is to say:
>>> import tomllib
>>> tomllib.loads('log.level = "DEBUG"')
{'log': {'level': 'DEBUG'}}
Now that configurations are dictionaries, we can construct them declaratively.
Add tomli as a backport of the standard library's new tomllib. By the way, it's not just a convention to use the `.toml` extension, it's actually part of the v1.0.0 spec: > TOML files should use the extension `.toml`. So we don't need to do any additional logic to determine whether a file is ini or toml -- if it doesn't have the right extension it literally isn't toml!
Add pytest-subtests plugin to test dependencies so that pytest will show
the parameters when a subTest fails.
Replace methods that check against specific combinations.
It would be a lot simpler to use pytest.mark.parametrize, which would
eliminate the need for pytest-subtests as well as the manual
setUp/tearDown, but that feature is unavailable for classes inheriting
from TestCase. Converting every class that inherits from ConfigTest to
pytest assertions would be too much extra noise to add to this branch,
which already has a large diff. I'm also not sure it's worth the effort
since it seems like it will take a couple more decades for python to
standardize a testing API. ("setup.py is deprecated; use pyproject.toml
and pick one of the 5 incompatible formats; use pytest as a test runner;
but unittest is in the standard library with a partially incompatible
API...")
Add toml-w development dependency so we can write toml in tests.
If we're going to degrade the validation and drop the documentation, the best favor we can do users is to deprecate ini configuration altogether as it is not receiving first-class support. I took this opportunity to remove the ini example configuration file from the table of contents and replace it with an extremely brief example at the beginning of the configuration documentation. In my opinion this is a long overdue change because a 200+ line configuration file is a poor excuse for documentation. If there's anything about the configuration format it covers that the documentation otherwise does not, I'm sure we can express it in less than 200 lines.
Add a new subcommand to automatically convert bugwarriorrc to bugwarrior.toml. This commit adds the ini2toml package as a dependency and implements a custom profile to handle the idiosyncrasies of our bugwarriorrc format. In my opinion this makes the transition easy enough that we no longer need to document bugwarriorrc at all so I went ahead and dropped what little remained. The only "bug" I've noticed so far with ini2toml is that empty lines are not preserved. This is the standard behavior of ini2toml due to the `normalise_newlines` postprocessor, which removes all empty lines and adds one before each section. However, this seems to be due to the issue that upstream tomlkit adds lots of arbitrary empty lines (aka `Whitespace()` and leaving them all would be an even worse result. See python-poetry/tomlkit#48. This is about as far as I care to chase this bug down the rabbit hole at the moment.
This may be taking it a bit too far, but it does have its advantages. The error-prone parsing and translation of ini files is now centralized in one place which should make it easier to weed out the bugs and has the additonal benefit of removing ini-specific code from the generic configuration loading module. It also brings back the prefix validation errors which had to be removed from the schema itself.
|
Closing in favor of #983. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #980 and #981.The overall goal was to complete #873, but I ended up carrying it further to the point of deprecating ini format in favor of toml. My main reason for wanting to do so is that I don't think there's currently any point to the service "prefixes", it introduces a lot of extra hacky meta-programming that is otherwise unneeded, and changing configuration file formats seems like a good opportunity to remove them. While this branch should be backwards compatible with INI format, the ini documentation and validation is severely degraded. I don't think it's practical to maintain backwards compatibility with first-class support, so better to just encourage everyone to jump to toml. Fortunately this is really easy with the
bugwarrior ini2tomlsubcommand that this branch adds.