Spring cleaning #788

obestwalter · 2018-04-14T17:01:17Z

see commit messages for details. This might grow a bit more over the next days, as I have some time at my hands and want to tidy up the code base a bit.

closes #797
closes #798
closes #799
closes #800
closes #801
closes #754

Also (no extra issues for that):

removed all experimental markers from docstrings (most of them have been there for a few years already). If they turn out wrong/bad, we just have to deprecate them and remove them in a major release
hookimpl is instantiated twice - mark one of them as deprecated and get rid of it later
moved hookimpl to __init__.py - having a hookimpl object in the hookspecs module is unnecessary and confusing. Access to hookimpl should happen as module attribute of tox (clearer where it comes from) - also kept a deprecated reference there for compatibilty.
removed some redundant rst references in hookspecs. If they are initialized once in any namespace they are defined globally (I don't like it either but that is how it works in restructuredText and ignoring that and defining the same things several times is confusing)
move _dummy object to only class where it is needed and name it like the constant that it is
use six for py2/3 compatibility where noticed it and where it is cheap to do
remove dead code
move exception related code into its own module rather than having it hang around in a class looking like a module in __init__.py
there is no need to have an intermediate run_main - that is used everywhere only under the name tox.cmdline rename function and use it directly, just like client code is using it
add a fixture for working in a clean tmpdir

* add reference to new cookiecutter * add a little tutorial with a concrete plugin to explain minimal reqs * add some explanations regarding naming

Sorry, this is a lot and it will get even more. I have some large chunks of time to spent with tox and I am going through with the big brush. I want to move towards a big clean up which will result in a tox 4 not to far into the future. These are the first steps. Part of this is getting stuff sorted for plugins to be more like 1. class citizens and stabilizing the API. A lot of things have been marked as experimental for many years now and it is time to let this go and rather deprecate things, if they should go away, instead of letting them hang around as experimental for ages. Start hookimpl/hookspec objects deprecation They are used in some places by importing direclty from tox. This might have been the right way a loing time ago, but now pluggy has an API to create those objects via pluggy.HookImplMarker and pluggy.HookSpecMarker without the need of importing them. hookimpl needs to be initiated for internal use and should be marked that way. Plugin writers should use the official API instead of importing it from tox. * removed all experimental markers from docstrings. If they turn out wrong/bad, we just have to deprecate them and remove them in a major release * also hookimpl is instantiated twice - mark these objects as deprecated and get rid of one of them later * especially having a hookimpl object in the hookspecs module is unnecessary and confusing * access hookimpl as module attribute of tox (clearer where it comes from) * removed some redundant rst references. If they are initialized once in any namespace they are defined globally (I don't like it either but that is how it works in restructuredText and ignroring that and defining the same things several times is confusing) tox-quickstart script and tests overhaul * never overwrite existing files - even if the user tries * make py36 the default environment rather than py27 if 1 is chosen * do not refer to the configuration as tox.ini as it might not be tox.ini but something else * refactor tests for readability and maintainability * adapt tests to new behaviour * refine some code having to do with usere input that turns into lists * remove unused flags from main * naming things (like, too terse or shadowing builtins, etc.) * whitespace (try to make it more consistent) * remove some redundant internal tests. Going through main is perfectly adequate and makes the tests more resilient to internal changes Other stuff * tox has an API already although this was never really planned - add comment to __all__ to make clear that this is the only 'official' API and that hookimpl and hookspec are not part of it * tidy up imports (prefer module imports to make clear where stuff comes from) * move _dummy object to only class where it is needed and name it like the constant that it is * use six for py2/3 compatibility where I notice it and where it is cheap to do * remove dead code * move exception related code into its own module rather than having it hang around in __init__.py * there is no need to have an intermediate run_main - that is used everywhere only under the name tox.cmdline rename function and use it directly, just like client code is using it * add a fixture for working in a clean tmpdir

It is used by plugin tests for tox-venv and it should not be marked private then. Added a note in the docstring about coordinating if this has to change Added a simple compat layer to tox-venv - this should be released soonish to be ready for that change in tox

obestwalter · 2018-04-14T17:02:35Z

[removed obsolete content. Relevant points are now in first post.]

asottile

reviewed commit-by-commit -- looks good! 👍

asottile · 2018-04-14T17:08:50Z

tox/_pytestplugin.py

@@ -34,6 +34,15 @@ def pytest_report_header():
    return "tox comes from: {}".format(repr(tox.__file__))


+@pytest.fixture
+def work_in_clean_dir(tmpdir):
+    old = tmpdir.chdir()


there's tmpdir.as_cwd() ctx for this

cool - thanks :)

codecov · 2018-04-14T17:10:53Z

Codecov Report

Merging #788 into master will decrease coverage by <.01%.
The diff coverage is 92.62%.

@@            Coverage Diff             @@
##           master     #788      +/-   ##
==========================================
- Coverage   94.85%   94.84%   -0.01%     
==========================================
  Files          11       13       +2     
  Lines        2408     2404       -4     
==========================================
- Hits         2284     2280       -4     
  Misses        124      124

Impacted Files	Coverage Δ
tox/_verlib.py	`83.15% <ø> (ø)`	⬆️
tox/__main__.py	`0% <0%> (ø)`	⬆️
tox/exception.py	`100% <100%> (ø)`
tox/interpreters.py	`92.85% <100%> (ø)`	⬆️
tox/_pytestplugin.py	`95.53% <100%> (-0.02%)`	⬇️
tox/venv.py	`96.55% <100%> (-0.02%)`	⬇️
tox/constants.py	`100% <100%> (ø)`
tox/config.py	`97.75% <100%> (-0.02%)`	⬇️
tox/hookspecs.py	`100% <100%> (ø)`	⬆️
tox/session.py	`94.75% <100%> (-0.02%)`	⬇️
... and 5 more

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 e603f78...e59afae. Read the comment docs.

rpkilby

Hey. I just did a quick once-over and haven't reviewed the code in depth. Just a few minor documentation suggestions.

rpkilby · 2018-04-14T19:28:50Z

doc/plugins.rst

-If you have a ``tox_MYPLUGIN.py`` module you could use the following
-rough ``setup.py`` to make it into a package which you can upload to the
-Python packaging index:
+You can create a new tox plugin with all bells and whistles via `Cookiecutter`_ (see `cookiecutter-tox-plugin <https://github.com/tox-dev/cookiecutter-tox-plugin>`_):


with all bells should be with all the bells

rpkilby · 2018-04-14T19:36:50Z

doc/plugins.rst

-If installed, the ``entry_points`` part will make tox see and integrate
-your plugin during startup.
+This will create a complete pypi-releasable, documented project with license, documentation,
+CI builds and other bells and whistles.


documented project with license, documentation,

Is a little redundant. "bells and whistles" has also been used recently Maybe...

This will create a complete, PyPI-ready package with a license, documentation, tests, and CI via Travis and Appveyor.

Definitely too many bells and whistles :)

rpkilby · 2018-04-14T19:43:19Z

doc/plugins.rst

+.. code-block:: console
+
+    $ cd </path/to/tox-fireworks>
+    $ python setup.py sdist register upload


The register command has been disabled and rolled into the upload command.

Should the instructions also include wheel & twine?

I added bdist_wheell, threw out the register thingy and added a hint to the Python Packaging Guide.

rpkilby · 2018-04-14T20:12:59Z

tox/_quickstart.py

+          '    - nosetests package.module\n'
+          '    - trial package.module\n')
+    do_prompt(map_, 'commands', 'Type the command to run your tests',
+              default='pytest', modificator=list_modificator)


Personally prefer to keep defaults/recommendations to the standard library. Maybe python -m unittest discover?

Good idea - I also thought that nose is not really alive and well anymore for a long time, so I will replace it with your suggested unittest call.

gaborbernat · 2018-04-15T10:45:44Z

doc/plugins.rst

@@ -85,8 +85,10 @@ it like:
 .. code-block:: console

    $ cd </path/to/tox-fireworks>
-    $ python setup.py sdist register upload
+    $ python setup.py sdist bdist_wheel upload



there's a typo in this commit message, can be left as it is if you do a squash merge, otherwise I suggest a reword 😄

Imprve definitely needs improving :)

gaborbernat · 2018-04-15T10:47:27Z

tests/test_z_cmdline.py

@@ -8,6 +8,7 @@
 import pytest

 import tox
+import tox.exc


not too happy with this exc, can we make it something more descriptive?

yep - we can have a module with the full name instead I think.

gaborbernat

another thing we might want to consider, now we have everything public here, if we make an official API should we maybe move to the private module approach of pytest? and have a root level __init__.py where we expose explicitly whatever we consider public?

gaborbernat · 2018-04-15T10:47:53Z

tox/__init__.py

 from pkg_resources import DistributionNotFound
 from pkg_resources import get_distribution

-from .hookspecs import hookimpl
+from .exc import exception  # noqa


why is the noqa needed here?

dunno, might not be necessary at all, but given that I will do some work on exception now I'll check.

gaborbernat · 2018-04-15T10:49:42Z

tox/__init__.py

+
+"""
+
+# DEPRECATED - will go away in tox 4


can we add some proxy thing for this to emit deprecation warning if imported instead of the deprecated comments? https://docs.python.org/2/library/exceptions.html#exceptions.DeprecationWarning

that way people using it will get some runtime noise to notify them 😄

I had the same idea already, but was not immediately sure how to accomplish that given a module level object. I will definitely look into that. I want to wait first though for input from @RonnyPfannschmidt as he made a comment that needs further clarification (tox-dev/detox#12 (comment)). It might be that I got this part completely wrong and it needs a rework and undoing of a lot of PRs I opened - I really hope not, but life is unfair sometimes :D

so found out https://www.python.org/dev/peps/pep-0562/ is for this, also shows a workaround for python 2 compatibility: Typical workarounds are assigning __class__ of a module object to a custom subclass of types.ModuleType or replacing the sys.modules item with a custom wrapper instance.

gaborbernat · 2018-04-15T10:51:18Z

tox/exc.py

+    return str_
+
+
+class exception:


if we use this as a namespace, wouldn't be nicer to make it a module, and use it as a file instead? this class thingy breaks PEP-8 and seems un-pythonic

That is definitely much saner than what I did. Then we would have an exception module and the naming thing is also sorted.

gaborbernat · 2018-04-15T10:52:10Z

tox/result.py

-            dict = {}
-        self.dict = dict
-        self.dict.update({"reportversion": "1", "toxversion": toxver})
+    def __init__(self, result_dict=None):


do we have some pep-8 analyser to the project, how this gets through? (according to that really should be an empty line here)

having an empty line between class statement and first method or docstring is optional and in fact most classes do not have that space in tox (48 vs 0 to be exact in main code - spaces are only in test classes). I prefer not having it also - I am a bit stingy with my vertical whitespace. I also avoid it inside methods and functions and rather split functionality out if for some reason I feel the need to have blocks)).

I will fix the classes in the tests still containing classes for consistency.

gaborbernat

another thing we might want to consider, now we have everything public here, if we make an official API should we maybe move to the private module approach of pytest? and have a root level __init__.py where we expose explicitly whatever we consider public?

obestwalter · 2018-04-15T11:26:37Z

Thanks @gaborbernat! There are a lot of good points you made. I will address them over the course of today.

Regarding individual commit messages: I think there is going to be a lot of back and forth until things are right and I have to admit, when I am in this kind of mode I tend to do a lot of things at the same time, which shows in the cleanliness or rather lack of cleanliness of the commits. So I think once this is tidy we should definitely squash it. I will keep an extensive log of the changes and why we made them. Before merging, I will also have a look at the complete diff and make sure all important changes are explained.

* add another layer of testing that checks the generated config * naming things * replace nose example and test with unittest * simplify actual test and do more work in test helpers * add another function to be used by code and test to unify generated output

gaborbernat · 2018-04-23T16:43:17Z

Sure works for me 👌

* start using predefined markers (only what is already used atm) * remove skipif markers where unnecessary

obestwalter · 2018-04-28T09:56:33Z

Created some issues and changelog entries that will be closed as a side effect of this cleaning session. This should be it for this time I'd say.

obestwalter · 2018-04-28T10:24:34Z

There. I think I will merge this on monday and push it out as an rc right away, so that we can start merging and releasing "real" PRs soon again :)

gaborbernat

re-commented on few of the unaddressed issues; definitely this became a monster PR, content wise seems mostly okay though!

gaborbernat · 2018-04-30T08:31:45Z

CHANGELOG.rst

@@ -781,7 +781,7 @@ Improved Documentation
  location ({envtmpdir}/pseudo-home).  If an index url was specified
  a .pydistutils.cfg file will be written with an index_url setting
  so that packages defining ``setup_requires`` dependencies will not
-  silently use your HOME-directory settings or https://pypi.python.org/pypi.
+  silently use your HOME-directory settings or PyPi.


removed a link here?

my point was maybe we should have replaced it with http://pypi.org instead of removing it.

gaborbernat · 2018-04-30T08:32:24Z

MANIFEST.in

@@ -6,3 +6,6 @@ include setup.py
 include tox.ini
 graft doc
 graft tests
+
+global-exclude __pycache__


still unsure why we need these includes?

They are excludes :) because otherwise it is possible to pollute a release package with these files.

see my point in #754 (comment)

gaborbernat · 2018-04-30T08:32:41Z

MANIFEST.in

@@ -6,3 +6,6 @@ include setup.py
 include tox.ini
 graft doc
 graft tests


https://docs.python.org/2/distutils/sourcedist.html#commands

gaborbernat · 2018-04-30T08:33:45Z

setup.py

@@ -20,8 +20,8 @@ def has_environment_marker_support():
    try:
        v = pkg_resources.parse_version(setuptools.__version__)
        return v >= pkg_resources.parse_version('0.7.2')
-    except Exception as exc:
-        sys.stderr.write("Could not test setuptool's version: %s\n" % exc)
+    except Exception as e:


any thoughts on this?

gaborbernat · 2018-04-30T08:34:42Z

tests/test_interpreters.py

-
-@pytest.fixture
-def interpreters():
+from tox.interpreters import (


ping again on this :-)

gaborbernat · 2018-04-30T08:36:11Z

tests/test_venv.py

+def test_test_hashseed_is_in_output(newmocksession, monkeypatch):
+    seed = '123456789'
+    monkeypatch.setattr('tox.config.make_hashseed', lambda: seed)
+    mocksession = newmocksession([], "")


I beg to differ, given @hpk42 is not actively maintaining this anymore I would prefer us to have Pythonic names now, and yes that involves changing some conventions here and there.

gaborbernat · 2018-04-30T08:37:37Z

tox.ini

-commands = sphinx-build -d "{toxworkdir}/docs_doctree" doc "{toxworkdir}/docs_out" --color -W -bhtml
-           sphinx-build -d "{toxworkdir}/docs_doctree" doc "{toxworkdir}/docs_out" --color -W -blinkcheck
+commands =
+    sphinx-build -d "{toxworkdir}/docs_doctree" doc "{toxworkdir}/docs_out" --color -W -bhtml


seems to me like adds no value, other than increasing the number of lines and changes in this file; and makes it inconsistent with other lines aligning content left to the =

gaborbernat

re-commented on few of the unaddressed issues; definitely this became a monster PR, content wise seems mostly okay though!

obestwalter · 2018-04-30T09:26:09Z

HI @gaborbernat there are a few other comments that I can't answer on directly (don't know why, I just don't have a reply window there).

About the currently dominating no-case naming convention: I have no problem with it. Also: If you grep the code base for mocksession you have 418 hits, so I am not starting to use a different name when I touch this. Renaming them all to mock_session just adds another character, so why bother?

FWIW In my own projects I actually intentionally break the slug case naming convention for names (I use camelCase for everything that behaves like data (including properties) and slug_case for everything that behaves like a function).

About that import stuff: we should really automate this soonish with a pre-commit setting that does not create 10 lines of code for 10 imports from the same module and use it consistently. Hope some hero will finish that patch that is in the works for this. I am usually a friend of minimizing vertical space without impairing legibility, so this is why I format imports like this until some automatic formatter comes along - this way of formatting also works with automatic imports in PyCharm btw. Formatting with 4 spaces indent on next line instead of along wherever the first import starts on the import line looks more consistent to me and yields longer lines for the actual imports, so it usually needs the same amount of vertical space. I use parentheses instead of backslashes because I only use backslashes as line breaks if I really absoutely have to, which is AFAIK only with assert statement messages:

assert something, \
    "some looooooooooooooooooooooooooooooooooooong message"

obestwalter · 2018-04-30T09:44:40Z

And the last thing:

I prefer

deps = 
    foo
    bar
    baz

over

deps = foo
       bar
       baz

for the same reason I prefer

from bla import (
    bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, 
    bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, 
    bla, bla, bla, bla, bla, bla, bla, bla)

over

from bla import (bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, 
                 bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, 
                 bla, bla, bla, bla, bla, bla, bla, bla, bla, bla, 
                 bla, bla, bla, bla)

When I adjust code working towards making it more consistent, then my muscle memory does these things, because that is how I am doing it for many years now. In the long run things like this should be auto formatted anyway and if this happens I don't really care either way as long as it is consistent and does not waste space like e.g.:

from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla 
from bla import bla

gaborbernat

So I really-really-really don't like the way import look with parenthesis, or the line breaks, or the snake case breaking. But I see your hate towards the \ char, and can understand muscle memory so I suppose ok.

As for why bother with snake case renaming? cause it makes the code more readable.

obestwalter added 6 commits April 12, 2018 11:46

change default to using simple pytest call

b8e1c29

cleaned up the code a bit

c7dd288

Improve plugin docs

b9d44e5

* add reference to new cookiecutter * add a little tutorial with a concrete plugin to explain minimal reqs * add some explanations regarding naming

Merge branch 'tox-quickstart-updates' into spring-cleaning

7f4aeea

obestwalter self-assigned this Apr 14, 2018

obestwalter requested review from hpk42, gaborbernat and asottile April 14, 2018 17:01

obestwalter requested review from rpkilby and nicoddemus April 14, 2018 17:04

fix docs to use pluggy API instead of import

591532e

asottile approved these changes Apr 14, 2018

View reviewed changes

obestwalter added 2 commits April 14, 2018 19:11

fix test to use pluggy API instead of import

50d6230

use tmpdir right - thanks @asottile

0e05f36

rpkilby reviewed Apr 14, 2018

View reviewed changes

obestwalter added 3 commits April 14, 2018 22:51

fix redundant formulation - thanks @rpkilby

e2f97fc

Imprve packaging and release orientation - thanks @rpkilby

a80fe9f

missed a word

fb13e94

gaborbernat reviewed Apr 15, 2018

View reviewed changes

gaborbernat requested changes Apr 15, 2018

View reviewed changes

obestwalter added 2 commits April 15, 2018 13:43

improve docstrings

f179e5f

obestwalter added 6 commits April 25, 2018 10:55

add new tox framework classifier to example and rephrase the naming part

61498c2

remove unused links

bdb0d17

fix typo

2d1f2a2

fix formatting error

5a5b3dc

fix redundancy in title

9662011

fix API (hookspec is NOT part of API, but hookimpl is)

e3bba6b

obestwalter mentioned this pull request Apr 28, 2018

handle interpreter directives with long lengths #794

Merged

obestwalter added 9 commits April 28, 2018 09:50

some fixmes/todos about the pytestplugin

5058cc9

some fixmes/todos about the pytestplugin

a6c3ff6

add link to packaging guide

d136a4f

update marker handling

98cbcd9

* start using predefined markers (only what is already used atm) * remove skipif markers where unnecessary

remove docstrings (don't work like that) - they're redundant anyway

84c5884

add a doc example, more explanation and fix formatting

753fe55

add changelog entries

b5944ca

add a little constructor instead of building factors in namespace

40d2583

API: add docstring and mark elements appropriately

558000c

add changelog entries

1f995b3

obestwalter mentioned this pull request Apr 28, 2018

Add codecov settings #804

Merged

obestwalter added 2 commits April 28, 2018 15:26

fix import formatting

2683477

fix import formatting

e59afae

gaborbernat requested changes Apr 30, 2018

View reviewed changes

gaborbernat approved these changes Apr 30, 2018

View reviewed changes

obestwalter merged commit ff7143e into tox-dev:master Apr 30, 2018

obestwalter deleted the spring-cleaning branch April 30, 2018 12:44

Spring cleaning #788

Spring cleaning #788

Conversation

obestwalter commented Apr 14, 2018 • edited Loading

obestwalter commented Apr 14, 2018 • edited Loading

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 14, 2018 • edited Loading

Codecov Report

rpkilby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obestwalter Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

gaborbernat left a comment

Choose a reason for hiding this comment

obestwalter commented Apr 15, 2018 • edited Loading

gaborbernat commented Apr 23, 2018

obestwalter commented Apr 28, 2018

obestwalter commented Apr 28, 2018

gaborbernat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat left a comment

Choose a reason for hiding this comment

obestwalter commented Apr 30, 2018 • edited Loading

obestwalter commented Apr 30, 2018 • edited Loading

gaborbernat left a comment • edited Loading

Choose a reason for hiding this comment

obestwalter commented Apr 14, 2018 •

edited

Loading

obestwalter commented Apr 14, 2018 •

edited

Loading

codecov bot commented Apr 14, 2018 •

edited

Loading

obestwalter Apr 15, 2018 •

edited

Loading

obestwalter commented Apr 15, 2018 •

edited

Loading

obestwalter commented Apr 30, 2018 •

edited

Loading

obestwalter commented Apr 30, 2018 •

edited

Loading

gaborbernat left a comment •

edited

Loading