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

Validate and clean up pyvista.plotting namespace #4508

Merged
merged 6 commits into from
Jun 11, 2023

Conversation

banesullivan
Copy link
Member

Follow up to #4486 to validate the changes did not break from pyvista.plotting import <feature> imports. I added tests that validate against the namespace in the last release but I did remove things that I did not think were intentionally exposed (which was quite a bit of stuff because of previous * imports).

@github-actions github-actions bot added the maintenance Low-impact maintenance activity label Jun 9, 2023
@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #4508 (6cbe2a1) into main (b693bb5) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #4508      +/-   ##
==========================================
+ Coverage   95.66%   95.72%   +0.05%     
==========================================
  Files         107      107              
  Lines       21048    21054       +6     
==========================================
+ Hits        20135    20153      +18     
+ Misses        913      901      -12     

tests/namespace/test_plotting_namespace.py Outdated Show resolved Hide resolved
tests/namespace/test_plotting_namespace.py Outdated Show resolved Hide resolved

from pyvista.core.errors import PyVistaDeprecationWarning

namesapce_data = pathlib.Path(__file__).parent / 'namespace-plotting.txt'
Copy link
Member

Choose a reason for hiding this comment

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

Just a style nitpick, but it might be better form to put this block of code in a function which gets called in parametrize() later. Unlikely that this test module will be imported, but still having this code in the global scope is a bit weird.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tend to let these sort of things slide in testing modules as no matter what, this code will have to run immediately on import or not. Maybe I wait until all PRs affecting this namespace testing directory are finished since this same logic repeats itself in three files now and make this reading/parameterizing one function in the conftest

doc/source/api/plotting/index.rst Outdated Show resolved Hide resolved
doc/source/api/plotting/index.rst Outdated Show resolved Hide resolved
banesullivan and others added 2 commits June 10, 2023 15:34
Co-authored-by: Andras Deak <adeak@users.noreply.github.com>
Copy link
Member

@adeak adeak left a comment

Choose a reason for hiding this comment

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

LGTM.

@tkoyama010 tkoyama010 merged commit b4b66eb into main Jun 11, 2023
24 checks passed
@tkoyama010 tkoyama010 deleted the maint/namespace-plotting branch June 11, 2023 21:48
@banesullivan banesullivan mentioned this pull request Jun 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Low-impact maintenance activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants