TOML set_env file support #3478
Conversation
|
|
||
| def replacer(raw_: str, args_: ConfigLoadArgs) -> str: | ||
| if conf is None: | ||
| replaced = raw_ # no replacement supported in the core section |
There was a problem hiding this comment.
TBH this condition I've just taken from the INI correspondant... Pls double-check if it makes sense in TOML parsing context. Thanks.
There was a problem hiding this comment.
This does not make sense here. Please revert changes to this.
juditnovak
force-pushed
the
fix/toml_setenv_from_file
branch
2 times, most recently
from
10c8e9c to
7dc1f78
Compare
juditnovak
changed the title
set_env from fileset_env from file
tests/config/test_set_env.py
Outdated
| ("conf_type", "config"), | ||
| [ | ||
| ("ini", "[testenv]\npackage=skip\nset_env=file|A{/}a.txt\nchange_dir=C"), | ||
| ("toml", '[env_run_base]\npackage="skip"\nset_env="file|A{/}a.txt"\nchange_dir="C"'), |
There was a problem hiding this comment.
It would make more sense that this to be a dictionary key rather than we just unroll a string. Something like 'set_env = [ { file= "a.txt"}] `.
There was a problem hiding this comment.
Just to make sure that I'm following correctly. Could you pls confirm that the following is what you suggest?
INI:
The current syntax supported for tox.ini for set_env:
[testenv]
set_env=file|<path>
TOML:
Moving away from the file| syntax introduced for tox.ini, benefitting from embedded data structures instead such as:
[env_run_base]
set_env=[{file=<path>}]
While this would raise an ERROR in TOML:
[env_run_base]
set_env="file|<path>"
Is this the direction you would like to take? Not to support file| terminology when TOML configuration is used?
(Sorry, I understood that a PR was welcome for the issue as raised -- which my attempt was aiming to respond. Of course, I'm glad to modify the code alongside a different design, if preferred. I agree that the above solution is more elegant.)
There was a problem hiding this comment.
Yeah, with the TOML format we generally are trying to move away from embedding information into strings, and instead use the rich type system and structure of the TOML language instead.
|
You still plan to tackle this? |
|
@gaborbernat Yes I do. Sorry for the silence, I was unavailable for a couple of days. Updated PR should be available early next week. |
juditnovak
force-pushed
the
fix/toml_setenv_from_file
branch
from
6fdde68 to
81a6997
Compare
|
@juditnovak tag me when ready 👍 |
|
@gaborbernat Sorry for the delay, the PR is ready. I keep thinking that I'm kind unhappy with the solution in terms of separation of concerns -- as part of the INI/TOML processing is happening on the level of each loader ( In case I'm overlooking a different, more elegant solution, feel free to let me know, and I'm glad to adjust. |
|
|
||
| def replacer(raw_: str, args_: ConfigLoadArgs) -> str: | ||
| if conf is None: | ||
| replaced = raw_ # no replacement supported in the core section |
There was a problem hiding this comment.
This does not make sense here. Please revert changes to this.
juditnovak
force-pushed
the
fix/toml_setenv_from_file
branch
from
04b9290 to
e4cabe8
Compare
|
@gaborbernat The condition in question is removed (assuming that 'revert' meant removal of newly added code). |
| delay_replace = inspect.isclass(of_type) and issubclass(of_type, SetEnv) | ||
|
|
||
| def replacer(raw_: str, args_: ConfigLoadArgs) -> str: | ||
| reference_replacer = Unroll(conf, self, args) | ||
| try: | ||
| replaced = str(reference_replacer(raw_)) # do replacements | ||
| except Exception as exception: | ||
| if isinstance(exception, HandledError): | ||
| raise | ||
| msg = f"replace failed in {args_.env_name}.{key} with {exception!r}" | ||
| raise HandledError(msg) from exception | ||
| return replaced | ||
|
|
||
| exploded = Unroll(conf=conf, loader=self, args=args)(raw) | ||
| return self.to(exploded, of_type, factory) | ||
| refactoried = self.to(exploded, of_type, factory) | ||
| if delay_replace: | ||
| refactoried.use_replacer(replacer, args=args) # type: ignore[attr-defined] # issubclass(to_type, SetEnv) | ||
| return refactoried |
There was a problem hiding this comment.
I mean this file should not be changed at all 🤔
There was a problem hiding this comment.
Could you please elaborate on your preferred strategy, please?
I was following the INI logic keeping INI and TOML processing side-by-side. If a different logic is preferred, may I ask what you have in mind?
There was a problem hiding this comment.
Why do we need this processing for TOML? This is an ini specific stuff 🤔
There was a problem hiding this comment.
No, it's not. The replacer function is used within the set_env module, to determine the correct path, that may have substitutions inside.
Note that we are using the TOML-specific Unroll() call within the code block in question.
This is the reason why TOML-specific tests with file="{env:myenvfile}work.
This code change is addressing #3474, making sure that TOML config has the same support to apply
set_envfrom file, as INI configuration does.