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

Feature/add typechecking #51

Merged
merged 13 commits into from
Dec 4, 2018
Merged

Feature/add typechecking #51

merged 13 commits into from
Dec 4, 2018

Conversation

techalchemy
Copy link
Member

  • Massive performance gains
  • Type checking
  • Regression test for shims being removed
  • Add typehint coverate to full library
  • Update tox config
  • Fix shim removal to only remove shims under the same conditions as we
    would activate the finders in question
  • Better regexes
  • Regresses 2.7.15+ version parsing atm

- Add typehint coverate to full library
- Update tox config
- Fix shim removal to only remove shims under the same conditions as we
  would activate the finders in question

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
src/pythonfinder/models/path.py Outdated Show resolved Hide resolved
src/pythonfinder/models/python.py Outdated Show resolved Hide resolved
@techalchemy
Copy link
Member Author

The sheer number of bugs this caught is actually crazy.

- add reload capability to finders and system path
- make sure we remove all NoneTypes
- leave shims on the path if we don't detect pyenv/asdf
- remove shims when we detect respective installs via corresponding
  environment variables
- make parsing more efficient with defensive regression testing to make
  sure we parse all options

Signed-off-by: Dan Ryan <dan@danryan.co>
- Reformat and remove unused code

Signed-off-by: Dan Ryan <dan@danryan.co>
@techalchemy
Copy link
Member Author

hrm i've been struggling with caching / clearing the cache... on here...

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
.travis.yml Outdated Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@jxltom
Copy link
Collaborator

jxltom commented Dec 3, 2018

Check the CI, the result is very interesting. It looks like it is not problem with xdist. The tests are still failing in python34 and 35

@jxltom
Copy link
Collaborator

jxltom commented Dec 3, 2018

Finally I figured out why the tests are failing. It is because PythonVersion.from_path is trying to parse from name and then from absolute path (if former one fails). In some cases, such as python35 in Travis, the name is 3.5 and PythonVersion.parse will successfully parse it though patch will be None in this case. I think it is better to parse from path (via calling subprocess) and then from name, which can get as more info as possible.

Feel free to revert this commit if you don't like it. 😃

@techalchemy
Copy link
Member Author

No we definitely want to avoid that. We just need to handle name correctly.

I swapped them because I profiled with a flamegraph and subprocess calls are wildly slow. Swapping the ordering here cut the runtime in 1/9. It should just use subprocess if it thinks it has a python version but actually doesn’t t

@jxltom
Copy link
Collaborator

jxltom commented Dec 3, 2018

Oh, it is for the performance. I missed that. So we only need to handle the names better.

I reverted the commit :D

@techalchemy
Copy link
Member Author

I didn't indicate it anywhere :p I just benchmarked locally

image

Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Signed-off-by: Dan Ryan <dan@danryan.co>
Pipfile Show resolved Hide resolved
@techalchemy techalchemy merged commit 5b22962 into master Dec 4, 2018
@techalchemy techalchemy deleted the feature/add-typechecking branch December 4, 2018 06:05
techalchemy added a commit that referenced this pull request Mar 2, 2019
Signed-off-by: Dan Ryan <dan@danryan.co>
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.

3 participants