Rewrite substitution parser #2861

masenf · 2023-01-15T01:07:02Z

Fix issue tox 3.27.1 to 4.0.9, environment var substitution failing #2732 by rewriting the regex-based substitution expression parser with a recursive-style parser.
Substitutions are processed left to right, inner-most to outer-most.
After a replacement is made in a string, the replaced part of the string will NOT affect characters after it. the result of a replacement will never be used in further substitution parsing.
- On windows {/} will expand to a literal backslash \, which will NOT act as an escape for the next character.
Additional test cases and regression tests are added.
Migrate and update "Substitutions" section of the "Configuration" doc from tox 3

Thanks for contribution

Please, make sure you address all the checklists (for details on how see
development documentation)!

ran the linter to address style issues (tox -e fix)
wrote descriptive pull request text
ensured there are test(s) validating the fix
added news fragment in docs/changelog folder
updated/extended the documentation

The resulting escaped value should not then be subsequently used as an escape in future replacement rounds. Additional friends, like triple backslash, which should escape the following, and quad backslash, which should escape to two single backslashes and not escape the following characters. The Quad backslash test case also has escaped double backslash at the beginning and end of the string for more edge coverage.

explicit test case for issue tox-dev#2732 which affects {/} os.sep replacement on windows platform. test_replace_os_sep_subexp_regression WiP

When the replacement value is a backslash, it shouldn't escape the next part of the value. However, if the replacement contains a backslash, then it could affect the next value.

go back and forth between mututally circular env vars

When the env var name is itself given by another env var

walk through the expression character by character, recursively descending on left curly brace and breaking arguments along the way should allow for arbitrarily complex nested expressions that do NOT affect adjacent expressions. enabling recursive replacement could be allowed on a per-expression basis, but that is not currently done

test_replace: {toxinidir,} shouldn't be replaced with the trailing comma test_replace: add test case for escaped colon arg sep test_replace: additional cases for test_match Single capital letter variable Single backslash Backslash escape non-special character Windows path shouldn't be mangled

describe ini-style curly brace substitutions

masenf · 2023-01-15T01:15:45Z

This is a rather large, fiddly PR. The more review I can get on this the better 🙏.

gaborbernat · 2023-01-15T05:59:02Z

docs/changelog/2372.bugfix.rst

@@ -0,0 +1,7 @@
+Rewrite substitution replacement parser - by :user:`masenf`
+
+* ``\`` acts as a proper escape for ``\`` in ini-style substitutions


This is more a feature than bugfixe, as it changes behavior of the logic.

gaborbernat · 2023-01-15T06:00:39Z

src/tox/config/loader/ini/replace.py

-        replace_value = replace_reference(conf, loader, value, conf_args)
-    return replace_value
+class MatchError(Exception):
+    pass


Docstring instead of pass.

gaborbernat · 2023-01-15T06:03:44Z

tests/config/loader/ini/replace/test_replace.py

-        (r"{foo:{bar}}", (5, 9, "bar")),
-        (r"{\{}", (0, 3, r"\{")),
-        (r"{\}}", (0, 3, r"\}")),
+        ("[]", [MatchExpression([["posargs"]])]),


Oh my, because you've changed the tests makes this pr infinitely harder to review. How am I supposed to know you did not introduce a lot of breaking changes 😅

i'm realizing now that find_replace_part was actually a public API of the tox.config.loader.ini.replace module, so I don't know if I can just "replace" it like this (even though the only caller in setenv was updated).

I took some time tonight to try to reimplement the find_replace_part API on top of the new find_replace_expr recursive implementation, and it's definitely possible, but the MatchExpression needs to track position information for where in the original value the match was found to allow reconstruction of the(start, end, replacement) tuple.

with that shim in place, I'd be able to bring back the original tests with no modification (only new tests, or those with clear behavior changes) to hopefully improve the reviewability here.

the other option would be keeping the regex parser, or at least the find_replace_expr part as it is currently and leave these tests in place... even though the ini-loader would actually be using the new parser under the hood.

thanks for looking.

I'll accept this as is, for now; we don't have public API besides the plugin interface, but I ask you to stick around for any regression bug reports and help out with that please.

I ask you to stick around for any regression bug reports and help out with that please.

100% please ping me if anything comes through that I don't notice and I'll jump on it.

this is a change in behavior, not merely a bugfix changelog: mention API names added and removed from the replace module to help people who might be searching for these names from error messages, etc

a first step at tracking enough position information to recreate the find_replace_part API; this one also improves the typing and readability of the class, so that the functions don't have to pass the term_pos in a tuple return value.

This PR contains the following updates: | Package | Type | Update | Change | Pending | |---|---|---|---|---| | [tox](https://togithub.com/tox-dev/tox) ([changelog](https://tox.wiki/en/latest/changelog.html)) | dev | minor | `4.2.8` -> `4.3.0` | `4.4.2` (+7) | --- ### Release Notes <details> <summary>tox-dev/tox</summary> ### [`v4.3.0`](https://togithub.com/tox-dev/tox/releases/tag/4.3.0) [Compare Source](https://togithub.com/tox-dev/tox/compare/4.2.8...4.3.0) #### What's Changed - Add Documentation URL for quick access via PyPI web by [@kerel-fs](https://togithub.com/kerel-fs) in [https://github.com/tox-dev/tox/pull/2854](https://togithub.com/tox-dev/tox/pull/2854) - Document factors by [@stephenfin](https://togithub.com/stephenfin) in [https://github.com/tox-dev/tox/pull/2852](https://togithub.com/tox-dev/tox/pull/2852) - Rewrite substitution parser by [@masenf](https://togithub.com/masenf) in [https://github.com/tox-dev/tox/pull/2861](https://togithub.com/tox-dev/tox/pull/2861) #### New Contributors - [@kerel-fs](https://togithub.com/kerel-fs) made their first contribution in [https://github.com/tox-dev/tox/pull/2854](https://togithub.com/tox-dev/tox/pull/2854) **Full Changelog**: tox-dev/tox@4.2.8...4.3.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box  Co-authored-by: descope[bot] <descope[bot]@users.noreply.github.com>

rwols · 2024-04-12T10:41:55Z

It seems that this changed behavior for the TOX_ENV_NAME, TOX_WORK_DIR injected environment variables.

Before this PR, you could reference these variables in the commands section with {env:TOX_ENV_NAME}. But after this PR, these placeholders resolve to the empty string.

masenf added 9 commits January 14, 2023 16:02

test_replace_os_sep_before_curly

44996c0

explicit test case for issue tox-dev#2732 which affects {/} os.sep replacement on windows platform. test_replace_os_sep_subexp_regression WiP

test_replace_env_when_value_is_backslash: replacement yields \

9325d66

When the replacement value is a backslash, it shouldn't escape the next part of the value. However, if the replacement contains a backslash, then it could affect the next value.

test_replace_env_var_circular_flip_flop

87986ed

go back and forth between mututally circular env vars

test_replace_env_default_deep

20a6e6c

test_replace_env_var_chase: indirect refs

6b66f77

When the env var name is itself given by another env var

docs/config.rst: add substitutions sections

9329377

describe ini-style curly brace substitutions

masenf requested a review from gaborbernat as a code owner January 15, 2023 01:07

psf-chronographer bot added the bot:chronographer:provided label Jan 15, 2023

gaborbernat reviewed Jan 15, 2023

View reviewed changes

masenf added 3 commits January 14, 2023 23:42

cr: changelog - fix issue number and mark as feature

64b961e

this is a change in behavior, not merely a bugfix changelog: mention API names added and removed from the replace module to help people who might be searching for these names from error messages, etc

replace: MatchExpression clean up position tracking

3f66e3a

a first step at tracking enough position information to recreate the find_replace_part API; this one also improves the typing and readability of the class, so that the functions don't have to pass the term_pos in a tuple return value.

cr: MatchError: docstring

2231986

gaborbernat merged commit aff1d4d into tox-dev:main Jan 16, 2023

benhoyt mentioned this pull request Jan 16, 2023

Tox 4.3.0 breaks substitution for values from other sections #2863

Closed

mikelane mentioned this pull request Jan 16, 2023

Bump tox from 4.2.8 to 4.3.0 mikelane/reddit-get#133

Merged

masenf mentioned this pull request Jan 16, 2023

Fix regression with multiple env substitutions for the same key #2873

Merged

5 tasks

masenf deleted the rewrite-substitution-parser branch January 18, 2023 04:25

masenf mentioned this pull request Jan 19, 2023

Document substutition #2126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite substitution parser #2861

Rewrite substitution parser #2861

masenf commented Jan 15, 2023

masenf commented Jan 15, 2023

gaborbernat Jan 15, 2023

gaborbernat Jan 15, 2023

gaborbernat Jan 15, 2023

masenf Jan 15, 2023

gaborbernat Jan 16, 2023

masenf Jan 16, 2023

rwols commented Apr 12, 2024

		@@ -0,0 +1,7 @@
		Rewrite substitution replacement parser - by :user:`masenf`

		* ``\`` acts as a proper escape for ``\`` in ini-style substitutions

Rewrite substitution parser #2861

Rewrite substitution parser #2861

Conversation

masenf commented Jan 15, 2023

Thanks for contribution

masenf commented Jan 15, 2023

gaborbernat Jan 15, 2023

Choose a reason for hiding this comment

gaborbernat Jan 15, 2023

Choose a reason for hiding this comment

gaborbernat Jan 15, 2023

Choose a reason for hiding this comment

masenf Jan 15, 2023

Choose a reason for hiding this comment

gaborbernat Jan 16, 2023

Choose a reason for hiding this comment

masenf Jan 16, 2023

Choose a reason for hiding this comment

rwols commented Apr 12, 2024