Updates toml library

[kingbuzzman]

Depends on:

• Adds coverage #316
• Adds tests for the current state of the project when using TOML #324

---

• Updates toml lib
  • Uses the latest and greatest first, and fallbacks to tomli if its not there
• Increases baseline test coverage by 6%

[bw2]

not to be paranoid, but how do we know we can trust geekyeggo/delete-artifact ?

[kingbuzzman]

See the

Depends on: #316

on the PR description. There is code here that is actually part of the PR above. Which is why i want to merge it faster.

[bw2]

That PR also used geekyeggo - which looks fine - but in general we want to be security-conscious when using 3rd party tools / images.

[kingbuzzman]

This was fixed in the main PR #316 -- has nothing to do with this PR

[kingbuzzman]

Fully tested. Also increases the coverage from 85% (master) to 91%

[kingbuzzman]

Also, please. I need this for another project, can this be released as soon as possible? 1.7.2 or 1.8.0?

[Copilot]

Pull Request Overview

This PR updates the TOML library integration by using tomllib (with a fallback to tomli) and adds new tests to increase coverage for TOML configurations. Key changes include:

• Updating the TOML parser in configargparse to support newer libraries.
• Adding extensive tests for TOML parsing scenarios.
• Adjusting setup.py dependencies for TOML support.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
tests/test_configargparse.py	Added tests for various TOML parsing cases, though duplicate test names were noted.
setup.py	Updated extras_require to include the correct dependency for TOML.
configargparse.py	Updated TOML parsing to use tomllib with fallback to tomli, with an outdated comment.

Comments suppressed due to low confidence (2)

tests/test_configargparse.py:1824

• Duplicate test function name 'test_empty_section' found. Consider renaming one of them to clearly differentiate between the intended test scenarios.

def test_empty_section(self):

configargparse.py:523

• The comment referring to 'configparser' is outdated since the parser now uses tomllib (with a fallback to tomli). Please update the comment to reflect the current implementation for clarity.

# parse with configparser to allow multi-line values

[bw2]

Looks fine to me. @tristanlatr @tbooth @agyoungs any objections to this and doing another release?

[agyoungs]

Looks fine to me. @tristanlatr @tbooth @agyoungs any objections to this and doing another release?

Thanks for the ping. I'll have time in a day or two to look at this unless someone beats me to it

[tbooth]

The switch to the new tomllib with a fallback to tomli makes sense.

However, I'll note that this PR introduces a dependency on the pytest module, which was not necessary to run the test suite in the past. Also the new tests are written in the pytest style, using @pytest.fixture decorators and bare assert statements rather than self.assertEqual() and friends. This is at odds with all the existing tests.

Personally I find the pytest approach hard to read and at odds with the "explicit is better than implicit" maxim of Python.

I think if there is a decision to move to this style of tests it should be all or nothing, and not just introduced on-the-fly within this PR. Or else, these new tests could very easily be written without the need for pytest.

[kingbuzzman]

Speaking as a pytest dev member -- well you know my bias.. I'm very pro-pytest.

[bw2]

@kingbuzzman I appreciate @tbooth 's point, and looking at this again, I agree that we should avoid heterogeneous testing styles where possible. Would you be up for switching this to the same style as existing tests?

[tristanlatr]

I suspect this test should be checking the following instead. And yes it’s not how it works right now for some reason, I suspect there is a regression compared to what the code was supposed to do initially, this because the tests were not merged at the same time as the extra config parser classes. The idea behind this is that anything that is not a string is casted to string because argparse expect only strings and will later re-convert il the string to float or whatever type.

This problem has been discovered in #314 and I don’t had the time to troubleshoot that behavior yet.

Suggested change

	assert parser.parse(f) == {'key1': "toml1", 'key2': [1, 2, 3]}
	assert parser.parse(f) == {'key1': "toml1", 'key2': ['1', '2', '3']}

So adding tests is good but the code is kinda broken right now.

So what ide suggest is:

• first make the original tests pass,
• then switch the TOML parsing library, ensuring that no breaking changes are introduced.

[tbooth]

I'm sure I fixed this somewhere in my monster PR...

[tbooth]

Yes, I did. See here: c1e3f16

Rather that the current behaviour where each individual parser is responsible for converting the config file contents to strings, I made a single function which is called by all the config file parsers. Note that at the request of @bw2 I later renamed _tweak_value() to _process_config_entry() but the logic is the same.

[kingbuzzman]

While i disagree with the approach, I understand the issue.

For now, I'm going to set this to draft.

But this has nothing to do with my PR, I'm going to set this to draft and create a PR where I ONLY create tests for what we currently have now, then ANOTHER PR where i fix the recast issue, THEN we can merge this PR.

[tbooth]

I'm looking to put my money where my mouth is and re-write the tests without using pytest myself. It's a useful exercise as in the process I'm uncovering some issues. Specifically, if I comment out line 1844 in tests/test_configargparse.py (ie. the line that adds configargparse.TomlConfigParser(['section']), to the composite parser) then all the tests still pass.

That can't be right!

[tbooth]

Also, if I switch back to using the old import toml and run these new tests then all the tests still pass (if I change 'rb' to 'r' when opening the toml files in the tests), so what are we actually gaining from this change? I think we need a test or two to demonstrate why the old code is inadequate, not just to show that the new change doesn't break anything.