Quote path settings containing # and spaces #1698
Conversation
src/tox/config/__init__.py
Outdated
| @@ -390,7 +391,7 @@ def get(self, name, default=None): | |||
| return os.environ.get(name, default) | |||
| self._lookupstack.append(name) | |||
| try: | |||
| res = self.reader._replace(val) | |||
| res = self.reader._replace(val, unquote_path=False) | |||
There was a problem hiding this comment.
How quoted paths should appear in the setenv results is an aspect I havent explored yet, and would likely build on #1691
Despite stripping quotes as early as possible, I expect there are quite a few corner cases where the quotes are going to causes new complications, which is why I have ensured the quotes are only used when # is in the path, which already didnt work, so corner cases breaking isnt a regression. Adding to the quoted path chars is less clear, as that probably worked for many simple cases, and only failed in corner cases as seen at #924.
There was a problem hiding this comment.
Tests for setting paths in setenv have been added.
Spaces in paths also added.
Merging #1691 first would allow for more exploration of possible edge cases using escaping.
jayvdb
force-pushed
the
i763
branch
2 times, most recently
from
7ce627e
to
3ecdd5f
Compare
jayvdb
changed the title
| @@ -1895,7 +1915,7 @@ def getargvlist(cls, reader, value, replace=True): | |||
| current_command += line | |||
|
|
|||
| if is_section_substitution(current_command): | |||
| replaced = reader._replace(current_command, crossonly=True) | |||
| replaced = reader._replace(current_command, crossonly=True, unquote_path=False) | |||
There was a problem hiding this comment.
unquote_path=False is added liberally to aid reviewing, so all invocations of _replace(..) can be seen.
IMO they should removed before being merged.
src/tox/config/__init__.py
Outdated
| new_word = reader._replace(word, unquote_path=False) | ||
| new_word = reader._replace(new_word, unquote_path=False) | ||
| if not has_dual_quote: | ||
| new_word = new_word.replace("''", "'") |
There was a problem hiding this comment.
this has the potential for changing the behaviour of any existing tox.ini which used quotes and also had quotes inserted via substitution. The most obvious case would be an env var of ' and commands using the envvar and quotes. Then, does stripping dual quotes change the behaviour of those previous usages? In theory, this removal would be simply merging quoted strings. Need to think more about possible breakages.
There was a problem hiding this comment.
Ah, this simple replace would break \\''. Easy fixed with a re.
There was a problem hiding this comment.
\\'' now fixed. No test coverage yet. Other obscure oddities may still exist.
| @@ -1864,6 +1878,12 @@ def _replace_substitution(self, match): | |||
| val = self._substitute_from_other_section(sub_key) | |||
| if callable(val): | |||
| val = val() | |||
| if isinstance(val, PathBase): | |||
| val = str(val) | |||
| # XXX handle ' and " in paths | |||
There was a problem hiding this comment.
This has partially been done.
I was thinking of switching quoting char to avoid clashes with '. The quoting could even be done with an invalid filename char (e.g. non-printables), as long as it makes it through the regex engine.
| argv = conf.commands | ||
| assert argv[0] == ["cmd1", "hello"] | ||
|
|
||
| if sys.platform != "win32": |
There was a problem hiding this comment.
Noting windows specific behaviour is happening here, cutting the dir at #. May be a bug in test harness code, given that the next stanza is OK.
jayvdb
force-pushed
the
i763
branch
3 times, most recently
from
1a9f5ac
to
266b1ab
Compare
jayvdb
requested review from
asottile,
gaborbernat and
obestwalter
as code owners
| @@ -0,0 +1 @@ | |||
| Allow {/} to refer to os.sep. - by :user:`jayvdb` | |||
There was a problem hiding this comment.
This is #1701
Without this, the tests need to do hacks to convert / to \ on Windows. It gets messy because the bugs related to this are about real native paths, including constructed path based on preset paths.
|
Sure, but now that #1691 is merged (thx), I'd like to also explore how all of these chars appear in the commands subprocess runtime environment variables. |
gaborbernat
force-pushed
the
master
branch
2 times, most recently
from
58336d1
to
7c103c8
Compare
|
#1713 solves many of these issues, in as much as that |
|
@jayvdb this is merge conflicting. |
|
I'll close this for now until you can attend to the PR again. Once you've done we can reopen. |
This is limited to settings of type "path".
While it is limited to
#andshlexcould be added.Fixes #763 and #924
Contribution checklist:
(also see CONTRIBUTING.rst for details)
in message body
<issue number>.<type>.rstfor example (588.bugfix.rst)<type>is must be one ofbugfix,feature,deprecation,breaking,doc,misc<your username>"superuser."CONTRIBUTORS(preserving alphabetical order)