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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore TOX_SKIP_ENV filtering #2707

Merged
merged 9 commits into from Dec 15, 2022
Merged

Conversation

mgedmin
Copy link
Contributor

@mgedmin mgedmin commented Dec 14, 2022

This is still documented in docs/config.rst. The only missing thing is the reporting at verbosity level 2.

I feel bad about the missing reporting. Could someone help me figure it out?

Fixes #2698.

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

@mgedmin
Copy link
Contributor Author

mgedmin commented Dec 14, 2022

I feel bad about the missing reporting. Could someone help me figure it out?

I think I have it, but what on earth is "verbosity level 2"? tox -vv? i.e. should I use logging.debug() for this?

@mgedmin
Copy link
Contributor Author

mgedmin commented Dec 14, 2022

I am very confused about logging levels. tox --help says

verbosity:
  every -v increases, every -q decreases verbosity level, default INFO, map 0=CRITICAL|1=ERROR|2=WARNING|3=INFO|4=DEBUG|5=NOTSET

  -v, --verbose               increase verbosity (default: 2)
  -q, --quiet                 decrease verbosity (default: 0)

so level 2 is WARNING, and the default level is INFO, which is 3, but also the default level is 2, which is WARNING.

#2709

Anyway, I'm logging with .info(), and it's visible only if you pass a single -v to tox, which means the actual default is WARNING and the --help output lies to me?

Should I change the log level of this message to .warning() so it would match docs/config.rst?

→ did so

@@ -0,0 +1,2 @@
``TOX_SKIP_ENV`` environment variable now works again (it was unintentionally
Copy link
Member

Choose a reason for hiding this comment

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

Please wrap at 120 characters.

Copy link
Member

Choose a reason for hiding this comment

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

Also do you want to add your user name here?

- :by:`x`.

Comment on lines 77 to 80
ini = """
[tox]
env_list = py3{10,9},mypy
"""
Copy link
Member

Choose a reason for hiding this comment

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

You can include this in one line considering its simplicity.

monkeypatch.setenv("TOX_SKIP_ENV", "m[y]py")
ini = """
[tox]
env_list = py3{10,9},mypy
Copy link
Member

Choose a reason for hiding this comment

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

You can include this in one line considering its simplicity.

def __init__(self, state: State) -> None:
# needs core to load the default tox environment list
# to load the package environments of a run environments we need the run environment builder
# to load labels we need core + the run environment
self.on_empty_fallback_py = True
self._warned_about = set()
Copy link
Member

Choose a reason for hiding this comment

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

You can move line 111 here; no need to define and then initialize it here; just initializing it is enough

Comment on lines 333 to 334
tox_env_filter = os.environ.get("TOX_SKIP_ENV")
tox_env_filter_re = re.compile(tox_env_filter) if tox_env_filter is not None else None
Copy link
Member

Choose a reason for hiding this comment

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

This definition should happen in the constructor of this class, not every time on this call, no?

mgedmin and others added 7 commits December 14, 2022 18:39
This is still documented in docs/config.rst.  The only missing thing is
the reporting at verbosity level 2.

Fixes tox-dev#2698.
Logging this at DEBUG level (tox -vv) felt wrong to me, so I went with
INFO (tox -v).
tox --help says verbosity level 2 is WARNING.
As far as I can tell, during runtime there's only one EnvSelector
instance, so there's no danger of repeatedly warning about the same
skipped environment when .iter() gets called several times.

Fixes failing unit tests.  (Oops.  I only ran each test in isolation,
because the entire test suite is a bit slow.)
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat merged commit c9394e8 into tox-dev:main Dec 15, 2022
@mgedmin
Copy link
Contributor Author

mgedmin commented Dec 15, 2022

Ah, sorry, I meant to take another look at this but was too busy yesterday.

Thank you for finishing it!

@mgedmin mgedmin deleted the restore-tox-skip-env branch December 15, 2022 11:29
@@ -88,6 +93,8 @@ def register_env_select_flags(
add_to.add_argument("-m", dest="labels", metavar="label", help=help_msg, default=[], type=str, nargs="+")
help_msg = "factors to evaluate"
add_to.add_argument("-f", dest="factors", metavar="factor", help=help_msg, default=[], type=str, nargs="+")
help_msg = "exclude all environments selected that match this regular expression"
add_to.add_argument("--skip-env", dest="skip_env", metavar="re", help=help_msg, default="", type=str)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this even more than the environment variable!

descope bot added a commit to descope/django-descope that referenced this pull request Dec 29, 2022
This PR contains the following updates:

| Package | Type | Update | Change | Pending |
|---|---|---|---|---|
| [tox](https://togithub.com/tox-dev/tox) | dev | patch | `4.0.9` ->
`4.0.11` | `4.1.0` (+8) |

---

### Release Notes

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

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

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

#### What's Changed

- Show only default env list for tox config by default by
[@&#8203;gaborbernat](https://togithub.com/gaborbernat) in
[tox-dev/tox#2726
- NO_COLOR follows no-color.org logic instead of tox boolean by
[@&#8203;gaborbernat](https://togithub.com/gaborbernat) in
[tox-dev/tox#2727
- Restore TOX_SKIP_ENV filtering by
[@&#8203;mgedmin](https://togithub.com/mgedmin) in
[tox-dev/tox#2707

**Full Changelog**:
tox-dev/tox@4.0.10...4.0.11

### [`v4.0.10`](https://togithub.com/tox-dev/tox/compare/4.0.9...4.0.10)

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

</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:eyJjcmVhdGVkSW5WZXIiOiIzNC43NC4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzQuMCJ9-->

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.

tox 4 ignores TOX_SKIP_ENV
2 participants