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

Don't import all keyrings unless necessary #404

Merged
merged 2 commits into from Oct 30, 2019
Merged

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Oct 17, 2019

If a keyring is set in configuration or environment variables,
use that one and don't load all available keyrings.

Partially addresses #403.

If a keyring is set in configuration or environment variables,
use that one and don't load all available keyrings.

Partially addresses jaraco#403.
@jyn514
Copy link
Contributor Author

jyn514 commented Oct 30, 2019

@mitya57 this has been sitting around for a while, are there any changes I should make?

@mitya57
Copy link
Collaborator

mitya57 commented Oct 30, 2019

Sorry for the delay. Thinking more about this, filter() is lazy in Python 3, so it does not actually do any iteration when you assign its result to a variable.

Maybe you are still using Python 2? We no longer support it…

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 30, 2019

Filter is lazy, but get_all_keyrings() is eager and is called before it is passed to filter. I am using Python3, I can show you the time speedups of you want.

@jyn514
Copy link
Contributor Author

jyn514 commented Oct 30, 2019

If you want to keep the existing code structure, maybe get_all_keyrings could be turned into a generator so it is evaluated lazily.

@mitya57
Copy link
Collaborator

mitya57 commented Oct 30, 2019

That makes sense. Thank you! Maybe in future we should make get_all_keyring lazier too (and add the missing s to its name).

@mitya57 mitya57 merged commit a62c1c6 into jaraco:master Oct 30, 2019
@jyn514 jyn514 deleted the import-time branch October 30, 2019 14:38
jaraco added a commit that referenced this pull request Nov 30, 2019
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.

None yet

2 participants