-
-
Notifications
You must be signed in to change notification settings - Fork 535
[bug] Plugins can only inject envs as default but not additional #3510
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
Comments
@gaborbernat any idea what could be done here? |
Don't know. This would require more in depth thought, which I did not get to yet. In an ideal world we'd have on loaders something like extend, but then we'd need to come up with merge strategies that's not simple. So don't know what's a good design here. |
My feeling is that if only there was a way to extend the lists of env names in the core config hook, then those envs could be populated in the env config hooks as it's possible already. |
That seems odly specific to cover just the |
But this is specifically about adding envs. What are the other settings that influence this? |
I don't think it is. This is about extending a core configuration and not overriding it. |
Not really, I'm looking for a way to avoid using/referencing |
Actually, looking at tox/src/tox/session/env_select.py Lines 181 to 183 in a985171
|
Ugh… Nope. It's failing on the further validation, then. |
Yes, but these new environments are a side effect of the core configuration. I am not saying it's not reasonable to extend it. I'm saying that we shouldn't come up with a solution for just this, but rather a generic solution for all core configurations. |
Could you expand what you mean by all core config, then? Perhaps, I'm not scoping the terminology the same way... |
|
@gaborbernat is that basically what's in the |
Possible signature: @impl
def tox_extend_envs() -> Iterable[str]: ( Hook point: https://github.com/tox-dev/tox/blob/b244a59/src/tox/session/state.py#L21. This must be happening after https://github.com/tox-dev/tox/blob/b244a59/src/tox/config/cli/parse.py#L62. |
Over the course of having discoveries related to #3507, I realized that all the examples with injecting
MemoryLoader(env_list=env_list)
add envs as default.And so typing in
tox
would attempt running them:I don't want to influence whatever the end-user configures as
env_list
in their config, only providing additional envs that should be requested viatox r -e <env>
(or through the env var, or config explicitly).It seems to me like a gap in the plugin API. Having stared long enough at the source code, it seems that the discovery of additional envs is tightly coupled with the source objects. It's effectively happening @
tox/src/tox/session/env_select.py
Line 180 in a985171
And if we unwrap a few layers of indirection, that effectively does
state.conf._src._discover_tox_envs(state.conf.core)
.It looks like this currently means that adding a memory loader into the core config adds default envs that are processed separately when discovering all envs:
tox/src/tox/session/env_select.py
Line 172 in a985171
But adding “inactive” / “additional” envs would only be possible by monkey-patching the private
state.conf._src
object orstate.conf.__iter__()
.@gaborbernat should the plugin API just implement a
tox_discover_envs()
hook or a method on thestate
orconfig
objects for declaring additional environments?The text was updated successfully, but these errors were encountered: