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

Support installed packages #5

Closed
wants to merge 3 commits into from

Conversation

gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented Apr 1, 2020

Resolves #3.

@gaborbernat
Copy link
Contributor Author

@asottile I think this is now ready for merge and shipping 👍

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat force-pushed the installed-source branch 2 times, most recently from d7e43be to 6f1c291 Compare April 2, 2020 11:54
Copy link
Owner

@asottile asottile left a comment

Choose a reason for hiding this comment

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

didn't get to review the whole thing, but here's a first pass

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
covdefaults.py Outdated Show resolved Hide resolved
covdefaults.py Show resolved Hide resolved
covdefaults.py Outdated Show resolved Hide resolved
covdefaults.py Outdated Show resolved Hide resolved
tests/covdefaults_test.py Outdated Show resolved Hide resolved
Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat
Copy link
Contributor Author

Without being able to automatically set paths, for which upstream does not have an API 🤦‍♂ I'm no longer interested in pursuing this further. I was hoping to shrink my conf from 25 lines to 3, but this would mean I'd need to settle at 10, plus an external dependency.

@gaborbernat gaborbernat closed this Apr 2, 2020
@gaborbernat gaborbernat reopened this Apr 4, 2020
@gaborbernat
Copy link
Contributor Author

I live this open for now. Feel free to review it 🤷‍♂ further; I'll keep in the source thing though.

@gaborbernat gaborbernat force-pushed the installed-source branch 3 times, most recently from b006e7e to 760f5e1 Compare April 14, 2020 00:44
@gaborbernat
Copy link
Contributor Author

@asottile upstream released a fix for paths get/set_option, added a few fixes to fail when options cannot be satisfied; now it's ready for final review and release 👍

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@asottile
Copy link
Owner

I couldn't get this working with a simple example, so I tried to fix it -- here's my patch on top of your branch: https://github.com/asottile/covdefaults/pull/new/installed-source-asottile

I was validating this by using astpretty using this patch:

diff --git a/setup.cfg b/setup.cfg
index fa1ab4b..bb47bd5 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -20,6 +20,8 @@ classifiers =
     Programming Language :: Python :: Implementation :: PyPy
 
 [options]
+package_dir =
+    =src
 py_modules = astpretty
 python_requires = >=3.6.1
 
@@ -35,6 +37,7 @@ universal = True
 
 [coverage:run]
 plugins = covdefaults
+parallel = true
 
 [mypy]
 check_untyped_defs = true

and

mkdir src && git mv astpretty.py src

and then using the coverage commands:

$ coverage erase && coverage run -m pytest tests && coverage combine && coverage report
============================= test session starts ==============================
platform linux -- Python 3.6.9, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /home/asottile/workspace/astpretty
collected 27 items                                                             

tests/astpretty_test.py ..........................x                      [100%]

======================== 26 passed, 1 xfailed in 0.11s =========================
Name    Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------
---------------------------------------------------
TOTAL      99      0      0      0   100%

2 files skipped due to complete coverage.

@gaborbernat
Copy link
Contributor Author

@asottile your proposal seems to me that you just put things back to track the source tree and not the installed package.

@gaborbernat
Copy link
Contributor Author

Investigating this I don't think it's easy to do this reliably for the general case, so I'll retract my feature request. Realized this would only make sense with source layout and that might be too much specialization.

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.

Allow specifying the source to be an installed library, rather than local folder
2 participants