Skip to content

file| active in add_testenv_attribute for type "dict" #1694

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

Closed
jayvdb opened this issue Oct 18, 2020 · 6 comments
Closed

file| active in add_testenv_attribute for type "dict" #1694

jayvdb opened this issue Oct 18, 2020 · 6 comments
Labels
bug:normal affects many people or has quite an impact

Comments

@jayvdb
Copy link

jayvdb commented Oct 18, 2020

It looks like the new file| from #1668 is not restricted to arg type dict_setenv, but is also allowed in venv args type="dict".

I've done a fairly extensive search for plugins using type="dict" and come up blank, so this is probably not impacting real users, and it only adds new syntax so almost certainly doesnt break any existing users. I am more concerned about whether it should be added there, because after a few releases it becomes a breaking change to remove it. Better to remove it now if it doesnt belong there.

That PR also adjusted variables in _getdict to be more about envvars when it could be any type of "dict", albeit having a env-var-like syntax of "name = value".

It probably isnt hard to disable file| for type="dict", but it doesnt seem too harmful to keep it.

I notice that https://tox.readthedocs.io/en/latest/plugins.html#tox.config.Parser.add_testenv_attribute doesnt mention allowable types "float", "dict", "basepython", "space-separated-list", and "env-list". (I am guessing that dict_setenv is intentionally internal-only, and shouldnt be used by plugins).

If file| support is kept for "dict", that would be the area to document it.

@jayvdb jayvdb added the bug:normal affects many people or has quite an impact label Oct 18, 2020
@gaborbernat
Copy link
Member

While technically the logic for this could be used for type dict to the intent of it is to be used for environment variable files. As such, I'd make it not available for type dict until someone presents a valid use case this makes sense.

@jayvdb
Copy link
Author

jayvdb commented Oct 18, 2020

Just to clarify, file| currently works for "dict". It seems you're keen to remove that.

More broadly wrt environment variable files, as it loads using all of the tox env substitution logic, these files are very different from standard .env files. It is only "inlining" (transcluding) content from a file , and it happens to be broadly similar to .env files for trivial .env content. I am sure there is much variability in how various programs parse .env files, so this isnt a big deal, but it is worth noting that it will behave wildly different than other parsers (esp wrt curly braces in variables).

@gaborbernat
Copy link
Member

as it loads using all of the tox env substitution logic

This is currently an implementation detail, relying on the env substitution logic is not really supported.

@jayvdb
Copy link
Author

jayvdb commented Oct 18, 2020

relying on the env substitution logic is not really supported.

The docs say "Rules within the environment file are the same as within the setenv (same replacement and comment support)."

Is that not accurate?

Or are you creating distinction between "setenv" and all the extra voodoo.

@gaborbernat
Copy link
Member

I mean we should probably alter that doc to include only comment support...

@gaborbernat
Copy link
Member

I believe this now has been addressed in tox 4 and its documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:normal affects many people or has quite an impact
Projects
None yet
Development

No branches or pull requests

2 participants