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

DRAFT: Rewrite of the configuration system #1488

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

KaiSforza
Copy link

Massive rewrite of the config system using pydantic. Definitely not ready to merge, I'm still figuring out some stuff here, just wanted to see if there was any feedback, honestly. I'm still figuring out what could be done here for performance, but otherwise it really cleans up the validation stuff.

Moved everything away from the lazy-config stuff that was previously
used. Hatch was already parsing the whole toml file anyway, it didn't
make much sense to do lazy loading of the configuration.

I've tried to keep the interface that was used almost the same, though
it is a bit different internally. Instead of passing a `dict` the
classes take a set of keyword arguments. All of these are type-checked
with [pydantic][1]. This cleans the code in hatch.config.model up a lot,
removing tons of duplicated type checks for the different parts of the
configuration. Now it's all basically taken care of, including the use
of `typing.Literal`s for things like `mode` and `style`.

Incidentally, this also fixes the issue of the spinner being
un-configurable, there's part of this that adds it as a real config so
users can have the cool `dots8Bit` spinner.

[1]: https://github.com/pydantic/pydantic
@KaiSforza KaiSforza marked this pull request as draft May 14, 2024 08:13
@ofek
Copy link
Sponsor Collaborator

ofek commented May 16, 2024

I'm quite busy currently but I'm extremely interested in this and have been planning on switching to Pydantic eventually!

@KaiSforza
Copy link
Author

@ofek awesome, I was just messing around with this, it seems quite a bit slower from when I've been testing, but I went way overboard in some places, and there are probably better ways to do many of the things I've been thinking.

Most of the reason I ended up doing this stuff was just because I noticed we were already 'parsing' the full config file, so it didn't make sense to do like 99% of the deferred rendering if we could get some sane, fast defaults. The only ones that keep messing with me are the git ones, but that shouldn't be too hard to come up with a lazy method for that.

It also really messes with test_model.py, since without the lazy rendering the tests will all fail pretty fast and need to test for subsets of the dict, not just the whole thing (see these). I'll keep poking at it, but it's a huge thing to switch this with what I'm currently doing.

In the latest version I have locally I made BaseConfig inherit from collections.abc.MutableMapping for some ease of use later in the process, which definitely makes things a bit more familiar. (can do foo(**config) and get expected results).

Anyway, I'll push some more things up later today.

We have a mutable map so just del the publish key.

Since we can't put in bad data into the config, I used
an empby 'BaseConfig' object as the 'model'. This lets
the clid/config/test_restore.py tests pass.
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.

None yet

2 participants