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

Move docopt and yarg imports to local scope #253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

befeleme
Copy link

@befeleme befeleme commented May 7, 2021

I wonder, would you consider moving the imported libraries from global to local scope?

My motivation comes from pipenv which bundles pipreqs alongside with its dependencies (yarg and docopt), but it really uses only two functions (get_all_imports, get_pkg_names) which don't require them.
When moved, pipenv could stop bundling yarg and docopt.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 71.255% when pulling 8c0f54a on befeleme:imports into 90102ac on bndr:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 71.255% when pulling 8c0f54a on befeleme:imports into 90102ac on bndr:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 71.255% when pulling 8c0f54a on befeleme:imports into 90102ac on bndr:master.

@alan-barzilay
Copy link
Collaborator

@befeleme I'm not sure I understood your motivation, pipreqs is not a dependency of pipenv and I couldn't find anything that mentions pipreqs being bundled with pipenv. Could you provide a source regarding this?

@befeleme
Copy link
Author

Sure! See vendored dependencies of pipenv. It looks pipreqs is bundled alongside with its installable dependencies and used in this function. The two dependencies mentioned in the PR are only pulled to make pipreqs import correctly.

Regarding why - pipenv comes with so many bundled libraries, it's huge. I am currently testing pipenv's dependencies in Fedora, as we prefer to debundle and pull them directly. To be fair, removing two of them will not significantly reduce the size of pipenv, but it's feasible and reduces a drop of its complexity.

@alan-barzilay
Copy link
Collaborator

@befeleme I see, thank you for the detailed response!
pipreqs was unmaintained for a while and I'm the new (and afaik sole) maintainer. I have a few plans for this project but they aren't set in stone yet, at the moment I'm focusing on issue #252 and it may lead to a refactoring that could break up the pipreqs.py file into smaller ones. This would probably resolve your issue with yarg and docopt.
I don't know yet how or if this is going to work but I would rather wait for the next release to be more mature before considering merging this PR. Even if I merged your PR right now it wouldn't be available until the next release, so I thing there is no harm in waiting and thinking a bit more.

@befeleme
Copy link
Author

@alan-barzilay, thanks for taking time to look at this issue :) Your point is completely understandable to me. Good luck with the refactoring efforts!

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