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

[WIP] "requires" config improvements #1022

Merged
merged 5 commits into from
Sep 27, 2018
Merged

[WIP] "requires" config improvements #1022

merged 5 commits into from
Sep 27, 2018

Conversation

obestwalter
Copy link
Member

@obestwalter obestwalter commented Sep 27, 2018

If requirements are not satisfied the error is now represented by a custom exception and appropriately caught to make the error look like what it is: a misconfiguration of the environment rather than an error in tox.

so this:

tox
ERROR: MissingRequirement: Packages tox-venv, tox-travis need to be installed alongside tox in [...]/bin/python

rather than:

tox                                 (git)-[generic-project/master:613bf7ce2c38+! ]
requirement missing tox-venv
requirement missing tox-travis
Traceback (most recent call last):
  File "[...]/bin/tox", line 11, in <module>
    sys.exit(cmdline())
  File "[...]/lib/python3.7/site-packages/tox/session.py", line 41, in cmdline
    main(args)
  File "[...]/lib/python3.7/site-packages/tox/session.py", line 46, in main
    config = prepare(args)
  File "[...]/lib/python3.7/site-packages/tox/session.py", line 28, in prepare
    config = parseconfig(args)
  File "[...]/lib/python3.7/site-packages/tox/config.py", line 232, in parseconfig
    ParseIni(config, config_file, content)
  File "[...]/lib/python3.7/site-packages/tox/config.py", line 982, in __init__
    self.ensure_requires_satisfied(reader.getlist("requires"))
  File "[...]/lib/python3.7/site-packages/tox/config.py", line 1099, in ensure_requires_satisfied
    raise RuntimeError("not all requirements satisfied, install them alongside tox")
RuntimeError: not all requirements satisfied, install them alongside tox

The docs give explicit info now also, that require is not doing what you might expect if you draw an analogy to the require options in setuptools.setup().

Marked this wip as I am not sure whether we really need a new exception or how to deal in general with our internal exceptions. I feel we should catch them all on the upper level and output a friendly error rather than a traceback (but maybe this should be tackled in another PR).

* Add explicit explanation to docs that those requirements will not be
  installed (as opposed to e.g. install_requires in setup.py where
  those are installed for you)
* crash with a friendly error (what is required and where does it need to be installed) but no Traceback that makes the failing Requirements look like an error in tox
* introduce a new exception for that (not sure if sensible should be discussed maybe, maybe we can use MissingDependency)
@codecov
Copy link

codecov bot commented Sep 27, 2018

Codecov Report

Merging #1022 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1022      +/-   ##
==========================================
- Coverage   70.39%   70.36%   -0.03%     
==========================================
  Files          14       14              
  Lines        3486     3486              
  Branches      465      465              
==========================================
- Hits         2454     2453       -1     
  Misses        971      971              
- Partials       61       62       +1
Impacted Files Coverage Δ
src/tox/interpreters.py 82.73% <ø> (ø) ⬆️
src/tox/config.py 65.1% <100%> (-0.11%) ⬇️
src/tox/session.py 78.12% <100%> (ø) ⬆️
src/tox/exception.py 79.16% <100%> (+0.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2b2739...f1c1646. Read the comment docs.

@gaborbernat
Copy link
Member

Fair catch, however will be rendered not needed by #998. We can merge it in the meantime.

@obestwalter
Copy link
Member Author

ah - didn't see that one yet. Definitely a good way to go. I just started catching up properly today and did not really go through all the issues :)

@gaborbernat
Copy link
Member

Looks like an improvement on its own so I'll merge it.

@gaborbernat gaborbernat merged commit 85c09ce into tox-dev:master Sep 27, 2018
@obestwalter obestwalter deleted the requires-improvements branch September 29, 2018 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants