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

Entry Point Warnings #850

Merged
merged 34 commits into from
Mar 12, 2020
Merged

Entry Point Warnings #850

merged 34 commits into from
Mar 12, 2020

Conversation

jeff-hernandez
Copy link
Contributor

@jeff-hernandez jeff-hernandez commented Feb 20, 2020

Plugins can fail to load silently at entry points. This PR closes #841 by warning users when plugins do fail to load.

@codecov-io
Copy link

codecov-io commented Feb 20, 2020

Codecov Report

Merging #850 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #850      +/-   ##
==========================================
+ Coverage   98.14%   98.18%   +0.04%     
==========================================
  Files         117      119       +2     
  Lines       10813    10849      +36     
==========================================
+ Hits        10612    10652      +40     
+ Misses        201      197       -4
Impacted Files Coverage Δ
featuretools/__init__.py 82.35% <100%> (+16.83%) ⬆️
featuretools/tests/plugin_tests/test_plugin.py 100% <100%> (ø)
featuretools/tests/plugin_tests/utils.py 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40d346e...212624f. Read the comment docs.

Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

Can you add a test case that triggers this exception?

featuretools/__init__.py Outdated Show resolved Hide resolved
M  featuretools/tests/plugin_tests/featuretools_plugin/featuretools_plugin/__init__.py
M  featuretools/tests/plugin_tests/test_plugin.py
M  featuretools/tests/plugin_tests/utils.py
M  featuretools/tests/plugin_tests/featuretools_plugin/featuretools_plugin/__init__.py
docs/source/changelog.rst Outdated Show resolved Hide resolved
@jeff-hernandez jeff-hernandez requested a review from rwedge March 10, 2020 21:06
Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

Can we also test not setting the logging level to debug and confirming the stack trace is not displayed?

featuretools/__init__.py Outdated Show resolved Hide resolved
@jeff-hernandez
Copy link
Contributor Author

Yes, will add that test case.

@jeff-hernandez jeff-hernandez requested a review from rwedge March 11, 2020 21:11
Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

This looks good. From a user experience perspective, I don't think the warning mentions that they can see more info if logging is set to debug. We might want to mention that in the warning so they know that option exists.

rwedge
rwedge previously approved these changes Mar 11, 2020
@rwedge rwedge dismissed their stale review March 11, 2020 21:31

Accidentally approved it

@jeff-hernandez jeff-hernandez requested a review from rwedge March 12, 2020 15:33
featuretools/__init__.py Outdated Show resolved Hide resolved
@jeff-hernandez jeff-hernandez requested a review from rwedge March 12, 2020 18:00
Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

Looks good

@jeff-hernandez jeff-hernandez merged commit 40c740c into master Mar 12, 2020
@jeff-hernandez jeff-hernandez deleted the entry-points branch March 12, 2020 18:07
@frances-h frances-h mentioned this pull request Mar 27, 2020
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.

Add warning if entry point plugin fails to load
3 participants