Repository: tox-dev/tox
Pull request #694 (2017-12-01 22:20:17)

allow defining negative conditional factors with a leading !
Pull request created for discussion on tox-dev/tox issues #292 and #688.

Still missing a doc/changelog/contributors changes, but the implementation & tests should be ok.

Suggested discussion points:
- negated factor operator other than `!`?
- any missing tests?

jurko-gospodnetic: This is a replacement for the original pull request https://github.com/jurko-gospodnetic/tox/pull/1 created in the fix repo before all the requirement pull requests got merged.
gaborbernat: @jurko-gospodnetic the code style check failed
codecov[bot]: # [Codecov](https://codecov.io/gh/tox-dev/tox/pull/694?src=pr&el=h1) Report
> Merging [#694](https://codecov.io/gh/tox-dev/tox/pull/694?src=pr&el=desc) into [master](https://codecov.io/gh/tox-dev/tox/commit/68ea0305b9d0792f1783ee6d494174c1eaf91e54?src=pr&el=desc) will **increase** coverage by `0.03%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/tox-dev/tox/pull/694/graphs/tree.svg?width=650&token=DYodAwDCZ5&height=150&src=pr)](https://codecov.io/gh/tox-dev/tox/pull/694?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
+ Coverage   89.45%   89.49%   +0.03%     
==========================================
  Files          11       11              
  Lines        2371     2379       +8     
  Branches      402      405       +3     
==========================================
+ Hits         2121     2129       +8     
  Misses        175      175              
  Partials       75       75
```


| [Impacted Files](https://codecov.io/gh/tox-dev/tox/pull/694?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [tox/config.py](https://codecov.io/gh/tox-dev/tox/pull/694/diff?src=pr&el=tree#diff-dG94L2NvbmZpZy5weQ==) | `94.88% <100%> (+0.05%)` | :arrow_up: |
| [tox/venv.py](https://codecov.io/gh/tox-dev/tox/pull/694/diff?src=pr&el=tree#diff-dG94L3ZlbnYucHk=) | `92.52% <0%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/tox-dev/tox/pull/694?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/tox-dev/tox/pull/694?src=pr&el=footer). Last update [68ea030...5107db4](https://codecov.io/gh/tox-dev/tox/pull/694?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

jurko-gospodnetic: @gaborbernat - heh, yeah, as the pull request description said at the time: `and there might be fix-lint failures to clean up` ;)

Should be fixed now.
gaborbernat: @asottile not sure what the pyupgrade means, but it failed.
asottile: in this case it's probably rewriting the python<2.7 set call to a python2.7+ set comprehension
jurko-gospodnetic: @gaborbernat  - Ok, I updated the code with the changes you requested.

- take a look at `_is_negated_factor()` and `_base_factor_name()` functions and where they are used in `tox/config.py`
- added 2 introductory doc cleanup commits not directly related to the negated factor conditionals feature
- added negated factor conditional related changelog entry
- updated docs with negated factor conditional related info

I briefly played with implementing the included/excluded factor splitting using something like this:
```
        split = dict(itertools.groupby(raw, key=_is_negated_factor))
        included = set(split[False])
        excluded = set(split[True])
```
which makes the code more concise but potentially too cryptic. In the end kept it as two dict comprehensions. Feel free to tweak as you like it.

When merging this, I suggest keeping the initial doc cleanup commits separate, but I did not feel like opening a pull request just for those.
jurko-gospodnetic: Btw. all this condition checking actually leads very nicely to a more expressive grammar supporting:
- `,` for `or`
- `-` for `and`
- `!` for `not`
- and `(`/`)` parenthesis for grouping

but that change is not precluded by just adding the ! operator support so I did not want to get into that in this pull request.
jurko-gospodnetic: I'm not really sure what you want added to `config-v2.rst`. That document does not contain full specification of how factor conditions are checked, as exists in `config.rst`. It seems like there's a lot of technical debt there that would need to be paid before extra documentation for this feature can be added and I don't really think all that should be done as a part of this pull request.

Patch:
diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index ffb4aa24..461e090b 100644
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -27,6 +27,7 @@ Jannis Leidel
 Johannes Christ
 Josh Smeaton
 Julian Krause
+Jurko Gospodnetić
 Krisztian Fekete
 Laszlo Vasko
 Lukasz Balcerzak
diff --git a/changelog/292.feature.rst b/changelog/292.feature.rst
new file mode 100644
index 00000000..71a1b633
--- /dev/null
+++ b/changelog/292.feature.rst
@@ -0,0 +1,1 @@
+add support for negated factor conditions, e.g. ``!dev: production_log`` - by @jurko-gospodnetic
diff --git a/doc/config.rst b/doc/config.rst
index d57d5d2c..2e12fe50 100644
--- a/doc/config.rst
+++ b/doc/config.rst
@@ -636,34 +636,42 @@ special case for a combination of factors. Here is how you do it::
 
     [testenv]
     deps =
-        py34-mysql: PyMySQL     ; use if both py34 and mysql are in an env name
-        py27,py36: urllib3      ; use if any of py36 or py27 are in an env name
-        py{27,36}-sqlite: mock  ; mocking sqlite in python 2.x
-
-Take a look at first ``deps`` line. It shows how you can special case something
-for a combination of factors, you just join combining factors with a hyphen.
-This particular line states that ``PyMySQL`` will be loaded for python 3.3,
-mysql environments, e.g. ``py34-django15-mysql`` and ``py34-django16-mysql``.
-
-The second line shows how you use same line for several factors - by listing
-them delimited by commas. It's possible to list not only simple factors, but
-also their combinations like ``py27-sqlite,py36-sqlite``.
-
-Finally, factor expressions are expanded the same way as envlist, so last
-example could be rewritten as ``py{27,36}-sqlite``.
+        py34-mysql: PyMySQL     ; use if both py34 and mysql are in the env name
+        py27,py36: urllib3      ; use if either py36 or py27 are in the env name
+        py{27,36}-sqlite: mock  ; mocking sqlite in python 2.x & 3.6
+        !py34-sqlite: mock      ; mocking sqlite, except in python 3.4
+        sqlite-!py34: mock      ; (same as the line above)
+
+Take a look at the first ``deps`` line. It shows how you can special case
+something for a combination of factors, by just hyphenating the combining
+factors together. This particular line states that ``PyMySQL`` will be loaded
+for python 3.3, mysql environments, e.g. ``py34-django15-mysql`` and
+``py34-django16-mysql``.
+
+The second line shows how you use the same setting for several factors - by
+listing them delimited by commas. It's possible to list not only simple factors,
+but also their combinations like ``py27-sqlite,py36-sqlite``.
+
+The remaining lines all have the same effect and use conditions equivalent to
+``py27-sqlite,py36-sqlite``. They have all been added only to help demonstrate
+the following:
+
+- how factor expressions get expanded the same way as in envlist
+- how to use negated factor conditions by prefixing negated factors with ``!``
+- that the order in which factors are hyphenated together does not matter
 
 .. note::
 
     Factors don't do substring matching against env name, instead every
-    hyphenated expression is split by ``-`` and if ALL the factors in an
-    expression are also factors of an env then that condition is considered
-    hold.
+    hyphenated expression is split by ``-`` and if ALL of its non-negated
+    factors and NONE of its negated ones are also factors of an env then that
+    condition is considered to hold for that env.
 
-    For example, environment ``py36-mysql``:
+    For example, environment ``py36-mysql-!dev``:
 
-    - could be matched with expressions ``py36``, ``py36-mysql``,
+    - would be matched by expressions ``py36``, ``py36-mysql`` or
       ``mysql-py36``,
-    - but not with ``py2`` or ``py36-sql``.
+    - but not ``py2``, ``py36-sql`` or ``py36-mysql-dev``.
 
 
 Other Rules and notes
diff --git a/doc/drafts/extend-envs-and-packagebuilds.md b/doc/drafts/extend-envs-and-packagebuilds.md
index f53fc1d0..a4c73a19 100644
--- a/doc/drafts/extend-envs-and-packagebuilds.md
+++ b/doc/drafts/extend-envs-and-packagebuilds.md
@@ -18,7 +18,7 @@ Goal: drive building of packages and the environments needed to test them, exerc
 
 It should be possible to build other kinds of packages than just the standard sdist and it should also be possible to create different kinds of builds that can be used from different environments. To make this possible there has to be some concept of factorized package definitions and a way to match these factorized builds to environments with a similar way of matching like what is in place already to generate environments. sdist would for example would match to a "sdist" factor to only be matched against virtualenvs as the default.
 
-This could then be used to hae virtualenv, conda, nixos, docker, pyenv, rpm, deb, etc. builds and tie them to concrete test environments.
+This could then be used to have virtualenv, conda, nixos, docker, pyenv, rpm, deb, etc. builds and tie them to concrete test environments.
 
 To summarize - we would need a:
 
@@ -114,19 +114,35 @@ Illustrate how to exclude a certain package format for a factor:
 ```ini
 [tox]
 plugins=conda
-envlist={py27,py35}, py27-xdist
+envlist=py27,py35,py27-xdist
 
 [testenv]
-package_formats=sdist wheel conda
 commands = py.test
+package_formats=sdist wheel conda
 exclude_package_formats=        # new option which filters out packages
     py27-xdist: wheel
 ```
 
+or possibly using the negated factor condition support:
+
+```ini
+[tox]
+plugins=conda
+envlist=py27,py35,py27-xdist
+
+[testenv]
+commands = py.test
+package_formats=
+    sdist
+    !py27,!xdist: wheel
+    conda
+```
+
 Output of `tox --list`:
 
 ```
 (sdist) py27
+(wheel) py27
 (conda) py27
 (sdist) py35
 (wheel) py35
diff --git a/tox/config.py b/tox/config.py
index 2508c18e..3e3568b7 100755
--- a/tox/config.py
+++ b/tox/config.py
@@ -848,8 +848,8 @@ class parseini:
         factors = set()
         if section in self._cfg:
             for _, value in self._cfg[section].items():
-                exprs = re.findall(r'^([\w{}\.,-]+)\:\s+', value, re.M)
-                factors.update(*mapcat(_split_factor_expr, exprs))
+                exprs = re.findall(r'^([\w{}\.!,-]+)\:\s+', value, re.M)
+                factors.update(*mapcat(_split_factor_expr_all, exprs))
         return factors
 
     def make_envconfig(self, name, section, subs, config, replace=True):
@@ -912,9 +912,31 @@ def _split_env(env):
     return mapcat(_expand_envstr, env)
 
 
+def _is_negated_factor(factor):
+    return factor.startswith('!')
+
+
+def _base_factor_name(factor):
+    return factor[1:] if _is_negated_factor(factor) else factor
+
+
 def _split_factor_expr(expr):
+    def split_single(e):
+        raw = e.split('-')
+        included = {_base_factor_name(factor) for factor in raw
+                    if not _is_negated_factor(factor)}
+        excluded = {_base_factor_name(factor) for factor in raw
+                    if _is_negated_factor(factor)}
+        return included, excluded
+
+    partial_envs = _expand_envstr(expr)
+    return [split_single(e) for e in partial_envs]
+
+
+def _split_factor_expr_all(expr):
     partial_envs = _expand_envstr(expr)
-    return [set(e.split('-')) for e in partial_envs]
+    return [{_base_factor_name(factor) for factor in e.split('-')}
+            for e in partial_envs]
 
 
 def _expand_envstr(envstr):
@@ -1064,12 +1086,14 @@ class SectionReader:
 
     def _apply_factors(self, s):
         def factor_line(line):
-            m = re.search(r'^([\w{}\.,-]+)\:\s+(.+)', line)
+            m = re.search(r'^([\w{}\.!,-]+)\:\s+(.+)', line)
             if not m:
                 return line
 
             expr, line = m.groups()
-            if any(fs <= self.factors for fs in _split_factor_expr(expr)):
+            if any(included <= self.factors
+                    and not any(x in self.factors for x in excluded)
+                    for included, excluded in _split_factor_expr(expr)):
                 return line
 
         lines = s.strip().splitlines()


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 696fdbd5..8a4e0e4e 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -1376,11 +1376,20 @@ class TestConfigTestEnv:
                 a: dep-a
                 b: dep-b
                 x: dep-x
+                !a: dep-!a
+                !b: dep-!b
+                !x: dep-!x
         """
         conf = newconfig([], inisource)
         configs = conf.envconfigs
-        assert [dep.name for dep in configs['a-x'].deps] == ["dep-all", "dep-a", "dep-x"]
-        assert [dep.name for dep in configs['b'].deps] == ["dep-all", "dep-b"]
+        expected = ["dep-all", "dep-a", "dep-x", "dep-!b"]
+        assert [dep.name for dep in configs['a-x'].deps] == expected
+        expected = ["dep-all", "dep-b", "dep-!a", "dep-!x"]
+        assert [dep.name for dep in configs['b'].deps] == expected
+        expected = ["dep-all", "dep-a", "dep-x", "dep-!b"]
+        assert [dep.name for dep in configs['a-x'].deps] == expected
+        expected = ["dep-all", "dep-b", "dep-!a", "dep-!x"]
+        assert [dep.name for dep in configs['b'].deps] == expected
 
     def test_factor_ops(self, newconfig):
         inisource = """
@@ -1392,16 +1401,25 @@ class TestConfigTestEnv:
                 a,b: dep-a-or-b
                 a-x: dep-a-and-x
                 {a,b}-y: dep-ab-and-y
+                a-!x: dep-a-and-!x
+                a,!x: dep-a-or-!x
+                !a-!x: dep-!a-and-!x
+                !a,!x: dep-!a-or-!x
+                !a-!b: dep-!a-and-!b
+                !a-!b-!x-!y: dep-!a-and-!b-and-!x-and-!y
         """
         configs = newconfig([], inisource).envconfigs
 
         def get_deps(env):
             return [dep.name for dep in configs[env].deps]
 
-        assert get_deps("a-x") == ["dep-a-or-b", "dep-a-and-x"]
-        assert get_deps("a-y") == ["dep-a-or-b", "dep-ab-and-y"]
-        assert get_deps("b-x") == ["dep-a-or-b"]
-        assert get_deps("b-y") == ["dep-a-or-b", "dep-ab-and-y"]
+        assert get_deps("a-x") == ["dep-a-or-b", "dep-a-and-x", "dep-a-or-!x"]
+        assert get_deps("a-y") == ["dep-a-or-b", "dep-ab-and-y",
+                                   "dep-a-and-!x", "dep-a-or-!x",
+                                   "dep-!a-or-!x"]
+        assert get_deps("b-x") == ["dep-a-or-b", "dep-!a-or-!x"]
+        assert get_deps("b-y") == ["dep-a-or-b", "dep-ab-and-y", "dep-a-or-!x",
+                                   "dep-!a-and-!x", "dep-!a-or-!x"]
 
     def test_envconfigs_based_on_factors(self, newconfig):
         inisource = """
@@ -1410,6 +1428,9 @@ class TestConfigTestEnv:
                 a: something
                 b,c: something
                 d-e: something
+                !f: something
+                !g,!h: something
+                !i-!j: something
 
             [unknown-section]
             some-setting=
@@ -1424,7 +1445,7 @@ class TestConfigTestEnv:
         config = newconfig(["-e py3-spam"], inisource)
         assert not config.envconfigs
         assert config.envlist == ["py3-spam"]
-        for x in "abcde":
+        for x in "abcdefghij":
             env = "py3-{}".format(x)
             config = newconfig(["-e {}".format(env)], inisource)
             assert sorted(config.envconfigs) == [env]