Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved factor selection to allow multiple uses of -f for "OR" and to allow hyphenated factors #2786

Merged
merged 9 commits into from Dec 29, 2022

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Dec 28, 2022

Summary

To enable better factor selection, this PR applies two changes. (resolves #2766)

  1. -f can now be passed multiple times. Envs matching any -f usage are selected (OR) on top of the existing behavior that envs must match all factors in a given usage (AND).

  2. -f will now split factors with hyphens.

(1) is the key functional change. (2) is not strictly necessary, but makes usage conform a little better to user expectations (or at least the expectations of one user! 馃憢 馃槈 ).

Example Usage

Consider an env list as follows:

envlist = py3{10,9}-django2{0,1}{,-cov}

Prior to this change, the following usage was valid: tox l -f py39 django20 and would select py39-django20 py39-django20-cov.
After this change, the same selection can be expressed with tox l -f py39-django20. Furthermore, the following selection which was previously not possible can be expressed:

$ tox l -f py39-django20 -f django21-cov
py310-django21-cov
py39-django20
py39-django20-cov
py39-django21-cov

The groups of selections are combined with OR semantics, but the individual selections are combined with AND.

Implementation Notes

I separated these two features into two commits, anticipating that (2) might be contentious. I would like to keep it as I don't think it adds much complexity, but I'm ready and willing to pull it out if necessary.

The parsing/interpretation of factors is now separated out into a dedicated helper method, which I note most other CLI args do not have. It looks like the CLI code avoids breaking things apart into too many small functions, but I thought this case benefits enough in terms of readability to be worth it. Again, I'm happy to change this in response to feedback.

The help message is rather long for this now, and I'm still not sure it's fully descriptive. Perhaps a future FAQ on how to use factors and labels would help.

Checklist

  • 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

Previously, when `-f` was passed, it overwrote the last value. The
result was that `-f foo -f bar` was equivalent to only passing
`-f bar`. Under the new behavior, `-f foo -f bar` combines `foo` and
`bar` as selection criteria, using OR-semantics. Envs matching
`foo OR bar` will be selected.

The existing multi-value argument behavior for `-f` is retained, in
which `-f foo bar` means `foo AND bar`. The behaviors can be combined
to express a variety of environment selections which were not
previously possible in a single invocation. e.g. `-f foo bar -f baz`
meaning `(foo AND bar) OR baz`.

No existing tests fail, and the new behavior is checked by a new test.
The help message for `-f` is updated.
The existing parsing of factors allows multiple factors to be selected
by passing them as multiple arguments to the `-f` flag. For example,
`-f foo bar` to pass both `foo` and `bar` as factors. This can now be
passed equivalently using `-f foo-bar`. The meaning of this
usage is identical to `-f foo bar`.

A new test checks the behavior, and very closely mirrors the existing
`-f` selection test so that their outputs are exactly equivalent.
These three tests are nearly identical in structure, and rely upon the
same project configuration. Convert from three distinct test cases to
a single parametrized test.

Also apply pre-commit, which does some mild reformatting.
Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, fix the type checker issues.

docs/changelog/2766.feature.rst Outdated Show resolved Hide resolved
- use tuple instead of list for immutable data
- use `continue` and `break` to skip unnecessary loop iterations
tests/session/test_env_select.py Outdated Show resolved Hide resolved
@gaborbernat gaborbernat marked this pull request as draft December 29, 2022 02:44
- convert args from list[str] to tuple[str, ...]
- reformat str concat into a `.format()` usage
This check cannot be reached because it relies on an impossible
combination of factors and labels.
@sirosen
Copy link
Contributor Author

sirosen commented Dec 29, 2022

I think I've addressed the outstanding changes needed. If this is going to squash merge, I think it's ready. Otherwise, I'd like to rebase it to tidy the history a little bit.

@gaborbernat gaborbernat marked this pull request as ready for review December 29, 2022 05:20
@gaborbernat gaborbernat merged commit 6cdd99c into tox-dev:main Dec 29, 2022
26 checks passed
@sirosen sirosen deleted the improved-factor-selection branch December 29, 2022 13:29
descope bot added a commit to descope/django-descope that referenced this pull request Jan 12, 2023
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.0.19` -> `4.1.0` | `4.2.8` (+11) |

---

### Release Notes

<details>
<summary>tox-dev/tox</summary>

### [`v4.1.0`](https://togithub.com/tox-dev/tox/releases/tag/4.1.0)

[Compare
Source](https://togithub.com/tox-dev/tox/compare/4.0.19...4.1.0)

#### What's Changed

- docs(config): Fix minor typo by
[@&#8203;rpatterson](https://togithub.com/rpatterson) in
[tox-dev/tox#2785
- Update user_guide.rst by [@&#8203;jamwil](https://togithub.com/jamwil)
in
[tox-dev/tox#2787
- Improved factor selection to allow multiple uses of `-f` for "OR" and
to allow hyphenated factors by
[@&#8203;sirosen](https://togithub.com/sirosen) in
[tox-dev/tox#2786

#### New Contributors

- [@&#8203;rpatterson](https://togithub.com/rpatterson) made their first
contribution in
[tox-dev/tox#2785
- [@&#8203;jamwil](https://togithub.com/jamwil) made their first
contribution in
[tox-dev/tox#2787
- [@&#8203;sirosen](https://togithub.com/sirosen) made their first
contribution in
[tox-dev/tox#2786

**Full Changelog**:
tox-dev/tox@4.0.19...4.1.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.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC45OS4yIiwidXBkYXRlZEluVmVyIjoiMzQuOTkuMiJ9-->

Co-authored-by: descope[bot] <descope[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doc request: how to do explicit invocations with multiple factors
2 participants