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

[WIP] Native TOML config #3309

Closed
wants to merge 8 commits into from
Closed

[WIP] Native TOML config #3309

wants to merge 8 commits into from

Conversation

ziima
Copy link
Contributor

@ziima ziima commented Jul 14, 2024

This is a prototype solution for #999, see discussion there.

This branch will be rebased a lot.

@ziima ziima requested a review from gaborbernat as a code owner July 14, 2024 18:49
return f"{type(self).__name__}(path={self.path})"

def transform_section(self, section: Section) -> Section: # noqa: PLR6301
return IniSection(section.prefix, section.name)
Copy link
Member

Choose a reason for hiding this comment

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

We should not use the ini system at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I duplicated IniSection to TomlSection for the time being. I plan to do more clean up later on.

if in_section.prefix is not None: # no prefix specified, so this could imply our own prefix
yield IniSection(in_section.prefix, a_base)

def sections(self) -> Iterator[IniSection]:
Copy link
Member

Choose a reason for hiding this comment

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

It makes no sense why have anything to do with the ini system here.

This is experimental API! Expect things to be broken.
"""

CORE_SECTION = CORE
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why we should reuse the ini systems prefixes for both the core and testing environment.

@@ -10,7 +10,7 @@

def out_no_src(path: Path) -> str:
return (
f"ROOT: No tox.ini or setup.cfg or pyproject.toml found, assuming empty tox.ini at {path}\n"
f"ROOT: No tox.ini or tox.toml or setup.cfg or pyproject.toml found, assuming empty tox.ini at {path}\n"
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I agree with the need for a dedicated TOML file. I am currently -1 on adding that.

Copy link
Member

Choose a reason for hiding this comment

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

I am in favour of defaulting to pyproject.toml but still allowing user to define an alternative config file to load. Some might find a need for it (like CI/CD, or custom user modified one when they cannot merge their changes in the default one).

I would say that even with alternative filename, I would still expect it to follow the same sections as the pyproject.toml, just to keep code simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went through the docs for several non-build tools I know (excluded those which do not support pyproject.toml at all) to see if the support custom TOML file. Here are results:

Tool Custom TOML file Custom TOML root
mypy none
ruff ruff.toml none
bump-my-version .bumpversion.toml tool.bumpversion or bumpversion
isort none
black none
codespell none
coverage none
towncrier towncrier.toml tool.towncrier

I'd say custom TOML file is not that rare. Custom root in TOML file seems to vary.

)
assert custom_section["a"] == "b"


Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this long comments.

@ziima
Copy link
Contributor Author

ziima commented Jul 15, 2024

I've ditched TomlLoader and updated MemoryLoader instead. They were mostly the same.



class TomlSource(Source):
"""Configuration sourced from a toml file (such as tox.toml).
Copy link
Member

Choose a reason for hiding this comment

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

I do not want a dedicated toml, but should be inside pyproject.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I keep that in mind, I just didn't get to that yet.

Personally I'd like to have tox.toml anyway. Tox configurations may get quite complex and I tend to keep them in a separate file, just one in format with a (better) specification. Currently, tox configuration can be written in setup.cfg, but I don't remember a single project where I would see that. From a contributor point of view, when I see tox.ini I know that the project uses tox and I don't really have to look up how to run tests.

Choose a reason for hiding this comment

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

I'd prefer to have an option of separate TOML config file as well. If you don't like a dedicated file such as tox.toml, what about allowing to specify configuration file in env variable, such as TOX_CONFIG?

Copy link
Member

Choose a reason for hiding this comment

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

I don't like the environment idea 😃

@ziima
Copy link
Contributor Author

ziima commented Jul 27, 2024

I changed testenv in TOML to tool.tox.env as suggested in #999 (comment)

Currently supported pyproject.toml looks like

[tool.tox]
env_list = ['py311', 'py312', 'type']

# The default testenv is `testenv`.
[tool.tox.env.testenv]
deps = 'pytest'
commands = ['pytest tests']

[tool.tox.env.type]
deps = 'mypy'
commands = ['mypy src']

I also added support for native pyproject.toml. It broke ability to load legacy pyproject.toml using --conf though. To be fixed later.

I split BaseSection from Section to handle different structure of data within TOML. I'm not quite sure what to do with that at the moment.

I keep tox.toml support at least for tests during development. It's easier that way.

@gaborbernat
Copy link
Member

I think deps needs to be a list of strings. Also, I would like to rename the base environment name to something like testenv -> base.

@ziima
Copy link
Contributor Author

ziima commented Aug 11, 2024

Ad options: deps just needs a tweak to work, but there is an underlying issue regarding factors. I'm not yet sure how to handle factors in TOML. For deps, I could just do

deps = ["quality: ruff"]

but that is somewhat clumsy and it really doesn't work with non-string options, such as ignore_errors.

I'm starting to think the best approach would be to provide a subkey to explicitly define factors in TOML, such as

[[tox.deps]]
name = "ruff"
when = ["quality"]

[[tox.deps]]
name = "pip"
when = ["py312", "py311"]

[[tox.deps]]
name = "setuptools"

# or a shortcut of
[tox]
deps = ["ruff", "pip", "setuptools"]
# when none of the deps has custom factors.

Ad base environment name: I tried to respect the base option, which has testenv as default value. I haven't polish this part, but this is the goal for the moment.

@gaborbernat
Copy link
Member

gaborbernat commented Aug 11, 2024

IMHO we should do something similar to what hatch does for environment generation and factors. I am ok with having dedicated toml.

@gaborbernat gaborbernat marked this pull request as draft August 13, 2024 16:40
@gaborbernat
Copy link
Member

@ziima do you plan to finalize this?

@ziima
Copy link
Contributor Author

ziima commented Sep 7, 2024

I'm back from holiday, so I should be able to find some time to work on this again.

@gaborbernat
Copy link
Member

Great to hear so. And thank you for doing all the hard work.

@gaborbernat
Copy link
Member

Replaced by #3353

@gaborbernat gaborbernat closed this Oct 1, 2024
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.

4 participants