[#826] switch to source layout #827

gaborbernat · 2018-05-17T20:43:37Z

a POC of it at least

gaborbernat · 2018-05-17T20:44:19Z

tox.ini

@@ -115,4 +115,4 @@ include_trailing_comma = True
 force_grid_wrap = 0
 line_length = 99
 known_first_party = tox
-known_third_party = pkg_resources,pluggy,py,pytest,setuptools,six


@asottile I think this is a false positive of the isort plugin

yeah, it assumes a specific layout. PRs welcome, the missing bits are to set the application directories:

For example, here's how reorder_python_imports handles this:
https://github.com/asottile/reorder_python_imports/blob/794f0498340ab3adbef443431993f8a8b2046da7/reorder_python_imports.py#L405-L411

You'll want:

args: [--application-directories, src]

once seed-isort-config supports that

let me know if you want to fix this or if you want me to fix it

I'm caught up now with other stuff, so if you can fix it that's cool 👍

asottile · 2018-05-17T21:48:49Z

Here ya go:

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 8a1d348..3469cf6 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -6,9 +6,10 @@ repos:
       args: [--line-length=99, --safe]
       python_version: python3.6
 -   repo: https://github.com/asottile/seed-isort-config
-    rev: v0.1.0
+    rev: v1.0.0
     hooks:
     -   id: seed-isort-config
+        args: [--application-directories, src]
 -   repo: https://github.com/pre-commit/mirrors-isort
     rev: v4.3.4
     hooks:
diff --git a/tox.ini b/tox.ini
index 548f602..9acd122 100644
--- a/tox.ini
+++ b/tox.ini
@@ -115,4 +115,4 @@ include_trailing_comma = True
 force_grid_wrap = 0
 line_length = 99
 known_first_party = tox
-known_third_party = pkg_resources,pluggy,py,pytest,setuptools,six,tox
+known_third_party = pkg_resources,pluggy,py,pytest,setuptools,six

codecov · 2018-05-17T23:10:17Z

Codecov Report

Merging #827 into master will decrease coverage by 4%.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #827    +/-   ##
======================================
- Coverage      95%    92%    -4%     
======================================
  Files          11     13     +2     
  Lines        2320   2414    +94     
  Branches        0    428   +428     
======================================
+ Hits         2212   2214     +2     
- Misses        108    124    +16     
- Partials        0     76    +76

obestwalter · 2018-05-25T23:11:22Z

tox.ini

@@ -103,7 +103,7 @@ source = tox
         {toxworkdir}/pypy*/site-packages/tox

 [pytest]
-addopts = -rsxX -vvv --showlocals
+addopts = -rsxX --showlocals


obestwalter · 2018-05-25T23:13:05Z

setup.py

@@ -1,7 +1,8 @@
 import io
 import sys

-import setuptools
+from setuptools import __version__ as setuptools_version


What is the connection to switching to src/ layout here? Is that some name shadowing thing?

nicoddemus · 2018-05-26T14:32:32Z

Rá, pytest wins the race! pytest-dev/pytest#3513 😸

gaborbernat · 2018-05-28T08:26:15Z

@nicoddemus the reason that's the case is cause I did not figure out yet how to merge coverage files in between interpreters, will probably do it tomorrow when I have some time for that. As I see pytest does not do that 🤔

anthrotype · 2018-05-28T10:06:25Z

you need to run coverage with --parallel-mode option and use [paths] source = ... in .coveragerc to list the file paths that should be considered equivalent when combining data:

https://coverage.readthedocs.io/en/coverage-4.5.1/config.html#paths

E.g.:
https://github.com/pyca/cryptography/blob/afdbfb13780fb78e7b277b9de07e7636ba9c5119/tox.ini#L18
https://github.com/pyca/cryptography/blob/afdbfb13780fb78e7b277b9de07e7636ba9c5119/.coveragerc#L7-L11

gaborbernat · 2018-05-28T12:02:30Z

@anthrotype I know but pytest-cov does some path mangling here and there which makes is more challenging, been down that path a few times.

anthrotype · 2018-05-28T12:41:31Z

@gaborbernat I see. I remember I also tried once to do it with pytest-cov but then gave up and decided to run pytest as coverage run -m pytest.

gaborbernat · 2018-06-10T09:55:12Z

@nicoddemus managed to nail down things now; tox is not too far behind! @obestwalter @asottile please review and we can merge 👍

nicoddemus · 2018-06-10T14:35:32Z

Awesome, great work! 😁

asottile · 2018-06-10T14:47:13Z

tox.ini

@@ -12,47 +12,60 @@ skip_missing_interpreters = true

 [testenv]
 description = run the tests with pytest under {basepython}
-setenv = COVERAGE_FILE={toxworkdir}/.coverage.{envname}
+setenv = VIRTUALENV_DOWNLOAD = 0


I've usually seen this written as VIRTUALENV_NO_DOWNLOAD=1

asottile · 2018-06-10T14:48:12Z

tox.ini

 passenv = http_proxy https_proxy no_proxy SSL_CERT_FILE TOXENV CI TRAVIS TRAVIS_* APPVEYOR APPVEYOR_* CODECOV_*
+deps = setuptools >= 29.0.0
+       setuptools_scm >= 2.0.0, < 3
+       wheel >= 0.31.0


you set --no-download,, but then ask for an upgrade of wheel / setuptools? (why not just allow virtualenv to do that for you at this point?)

It has to do with the index URL used. By default virtualenv uses PyPi. This way the tox config adheres too whatever the user has configured; e.g. value from PIP_INDEX_URL.

virtualenv respects PIP_INDEX_URL as well (it's actually just invoking pip under the hood)

either way, I don't think this boilerplate is helping you solve this issue so let's drop this or move it to another PR

asottile · 2018-06-10T14:49:09Z

tox.ini

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


 [testenv:fix-lint]
+description = run static analysis and style check using flake8


heh, description isn't exactly right

asottile · 2018-06-10T14:50:01Z

tox.ini

-description = combine coverage data and create reports
-deps = coverage
+description = [run locally after tests]: combine coverage data and create report;
+              generates a diff coverage against origin/master (can be changed by setting DIFF_AGAINST env var)


should DIFF_AGAINST be in passenv?

well spotted, fixed

asottile · 2018-06-10T14:50:42Z

tox.ini

 basepython = python3.6
 passenv = {[testenv]passenv}
          HOMEPATH
          # without PROGRAMDATA cloning using git for Windows will fail with an
          # `error setting certificate verify locations` error
          PROGRAMDATA
 extras = lint
-description = run static analysis and style check using flake8
+changedir = {toxinidir}


heh, pre-comit will cd right back to the repository root, not sure this is useful

asottile · 2018-06-10T14:51:13Z

tox.ini

@@ -90,21 +106,21 @@ max-line-length = 99
 ignore = E203, W503

 [coverage:run]
-omit = tox/__main__.py
+branch = false


branch coverage is useful! please don't turn it off!

asottile · 2018-06-10T14:55:15Z

tox.ini

+source = src/tox
+         .tox/*/lib/python*/site-packages/tox
+         .tox/*/Lib/site-packages/tox
+         .tox/pypy*/site-packages/tox


this is by far my least favorite part of moving to a src directory, the coverage reports become unreadable :(

we've used this script or a derivative of it to try and make this more sane (it combines the coverage data from site-packages over top the local directory)

hmm, combining the data is still fine (e.g. tox -e py27,coverage); only the pytest run one is messed up; I would prefer to fix this upstream instead 👍

asottile

otherwise looks good

while you're at it, can probably add skip_install = True to the pre-commit env

asottile · 2018-06-10T20:39:09Z

tox.ini

 basepython = python3.6
 passenv = {[testenv]passenv}
          HOMEPATH
          # without PROGRAMDATA cloning using git for Windows will fail with an
          # `error setting certificate verify locations` error
          PROGRAMDATA
+          DIFF_AGAINST


oops, added this to the wrong testenv, needs to be in the coverage testenv

gaborbernat · 2018-06-11T09:10:24Z

things now look great 👍 so I'll go ahead and merge it. 🙊 sorry for people with PRs for the merge conflicts

gaborbernat requested a review from obestwalter May 17, 2018 20:43

gaborbernat commented May 17, 2018

View reviewed changes

gaborbernat force-pushed the master branch from f49a0c8 to 3563d45 Compare May 17, 2018 22:33

obestwalter reviewed May 25, 2018

View reviewed changes

gaborbernat force-pushed the master branch 2 times, most recently from c42c7dc to 0580fe2 Compare June 10, 2018 08:39

move to src layout

06699c0

gaborbernat force-pushed the master branch 4 times, most recently from 37c55d0 to 911e704 Compare June 10, 2018 09:42

fix coverage

3c17955

gaborbernat force-pushed the master branch from 911e704 to 3c17955 Compare June 10, 2018 09:47

gaborbernat requested review from obestwalter, nicoddemus and asottile June 10, 2018 09:54

asottile reviewed Jun 10, 2018

View reviewed changes

gaborbernat requested a review from asottile June 10, 2018 20:31

asottile approved these changes Jun 10, 2018

View reviewed changes

gaborbernat force-pushed the master branch from d312643 to f275ff4 Compare June 11, 2018 08:16

PR feedback

38a0768

gaborbernat force-pushed the master branch from f275ff4 to 38a0768 Compare June 11, 2018 08:35

gaborbernat merged commit 4e7d909 into tox-dev:master Jun 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#826] switch to source layout #827

[#826] switch to source layout #827

gaborbernat commented May 17, 2018

gaborbernat May 17, 2018

asottile May 17, 2018

asottile May 17, 2018

gaborbernat May 17, 2018

asottile commented May 17, 2018

codecov bot commented May 17, 2018 •

edited

Loading

obestwalter May 25, 2018

obestwalter May 25, 2018

nicoddemus commented May 26, 2018

gaborbernat commented May 28, 2018

anthrotype commented May 28, 2018 •

edited

Loading

gaborbernat commented May 28, 2018

anthrotype commented May 28, 2018

gaborbernat commented Jun 10, 2018 •

edited

Loading

nicoddemus commented Jun 10, 2018

asottile Jun 10, 2018

asottile Jun 10, 2018

gaborbernat Jun 10, 2018

asottile Jun 10, 2018

asottile Jun 10, 2018

asottile Jun 10, 2018

gaborbernat Jun 10, 2018

asottile Jun 10, 2018

asottile Jun 10, 2018

asottile Jun 10, 2018

gaborbernat Jun 10, 2018

asottile left a comment

asottile Jun 10, 2018

gaborbernat commented Jun 11, 2018 •

edited

Loading

[#826] switch to source layout #827

[#826] switch to source layout #827

Conversation

gaborbernat commented May 17, 2018

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

asottile commented May 17, 2018

codecov bot commented May 17, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented May 26, 2018

gaborbernat commented May 28, 2018

anthrotype commented May 28, 2018 • edited Loading

gaborbernat commented May 28, 2018

anthrotype commented May 28, 2018

gaborbernat commented Jun 10, 2018 • edited Loading

nicoddemus commented Jun 10, 2018

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

asottile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat commented Jun 11, 2018 • edited Loading

codecov bot commented May 17, 2018 •

edited

Loading

anthrotype commented May 28, 2018 •

edited

Loading

gaborbernat commented Jun 10, 2018 •

edited

Loading

gaborbernat commented Jun 11, 2018 •

edited

Loading