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.

Sign up for GitHub

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

Jump to bottom

RFC: encapsulate try blocks from on demand imports #3935

Merged
merged 2 commits into from
May 23, 2022
Merged

RFC: encapsulate try blocks from on demand imports #3935

merged 2 commits into from
May 23, 2022

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented May 20, 2022 •
edited
Loading

PR Summary

My latest attempt at making on_demand_imports.py less repetitive and error prone
I expect I'll need to iterate a couple times on this.

Gains:

  • developer experience: far less boilerplate required to add optional dependencies, and an overall lighter module
  • user experience: no more necessary startup overhead for some optional deps (like pandas and requests) who were imported even when not used.

TODO:

  • cleanup history
  • add a couple tests to check that using a missing import raises the expected error (currently I think this is not tested because we skip any test that requires deps that aren't available)

@neutrinoceros neutrinoceros added the refactor improve readability, maintainability, modularity label May 20, 2022
@neutrinoceros neutrinoceros force-pushed the upgrade_cached_properties_on_demand_imports branch 2 times, most recently from d4645e2 to 28233f3 Compare May 20, 2022 17:20
@neutrinoceros
ENH: avoid unnecessary startup overhead for available optional depend…
c501b8c 
…encies, reduce boilerplate code in on_demand_import.py
@neutrinoceros neutrinoceros force-pushed the upgrade_cached_properties_on_demand_imports branch from 61fd2d5 to 5085bf3 Compare May 20, 2022 21:57
@neutrinoceros neutrinoceros force-pushed the upgrade_cached_properties_on_demand_imports branch from 5085bf3 to defb07d Compare May 20, 2022 22:05
@neutrinoceros neutrinoceros marked this pull request as ready for review May 21, 2022 05:37
@neutrinoceros neutrinoceros mentioned this pull request May 21, 2022
Merged
3 tasks
@matthewturk
Copy link
Member

Looks good to me

@matthewturk matthewturk enabled auto-merge May 23, 2022 14:10
@matthewturk matthewturk merged commit bb87cb8 into yt-project:main May 23, 2022
@neutrinoceros neutrinoceros deleted the upgrade_cached_properties_on_demand_imports branch May 23, 2022 14:17
@neutrinoceros neutrinoceros mentioned this pull request Jun 9, 2022
Closed
@neutrinoceros neutrinoceros mentioned this pull request Jul 13, 2022
Closed
neutrinoceros added a commit to neutrinoceros/yt that referenced this pull request Jul 13, 2022
@neutrinoceros
TMP: completely revert GH yt-project#3935
e177a8f
@neutrinoceros neutrinoceros mentioned this pull request Jul 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewturk matthewturk matthewturk approved these changes

Assignees
No one assigned
Labels
refactor improve readability, maintainability, modularity
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@neutrinoceros @matthewturk