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

Split out builtins into another top-level module #4706

Merged
merged 29 commits into from
Aug 2, 2022

Conversation

tlambert03
Copy link
Contributor

Description

This PR would split our "builtins" plugins into a new top-level module napari_builtins. In this PR, it remains distributed with napari package on PyPI. If we want to go in this direction (as has been discussed) i think it would be good to have at least a little period of co-existence of the two packages in the same repo, since I think there will likely be some follow-ups. Particularly with regards to testing: we currently do a lot of sorta-kinda indirect integration testing and it would be a good opportunity to clean up our tests to clarify whether we're testing how napari calls plugins, vs how our builtin plugin works.

i think most tests are passing currently, but still in draft cause I want to go back and clean up the tests even more

Type of change

  • Bug-fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

References

How has this been tested?

  • example: the test suite for my feature covers cases x, y, and z
  • example: all tests pass with my change
  • example: I check if my changes works with both PySide and PyQt backends
    as there are small differences between the two Qt bindings.

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added qt Relates to qt tests Something related to our tests labels Jun 17, 2022
@github-actions github-actions bot added the task Tasks for contributors and maintainers label Jun 18, 2022
@tlambert03 tlambert03 mentioned this pull request Jun 18, 2022
12 tasks
@Czaki
Copy link
Collaborator

Czaki commented Jun 21, 2022

when we move part of code to new top level package, then maybe new entry should be added to coverage.status.project in coverage.yaml?

@tlambert03 tlambert03 marked this pull request as ready for review June 22, 2022 12:32
@tlambert03 tlambert03 changed the title [WIP] Split out builtins into another top-level module Split out builtins into another top-level module Jun 22, 2022
@codecov
Copy link

codecov bot commented Jul 23, 2022

Codecov Report

Merging #4706 (a63c58f) into main (e1f6779) will decrease coverage by 0.08%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #4706      +/-   ##
==========================================
- Coverage   87.68%   87.59%   -0.09%     
==========================================
  Files         589      580       -9     
  Lines       50631    49715     -916     
==========================================
- Hits        44394    43549     -845     
+ Misses       6237     6166      -71     
Impacted Files Coverage Δ
napari/plugins/__init__.py 84.84% <0.00%> (+2.49%) ⬆️
napari/utils/io.py 78.94% <ø> (-10.58%) ⬇️
napari/_qt/_tests/test_qt_viewer.py 94.26% <100.00%> (ø)
napari/_qt/qt_viewer.py 71.39% <100.00%> (+0.63%) ⬆️
napari/components/_tests/test_viewer_labels_io.py 100.00% <100.00%> (ø)
napari/conftest.py 92.97% <100.00%> (+2.44%) ⬆️
napari/types.py 89.47% <0.00%> (-7.50%) ⬇️
napari/components/experimental/chunk/_pool.py 85.71% <0.00%> (-3.18%) ⬇️
napari/plugins/io.py 50.00% <0.00%> (-2.55%) ⬇️
napari/components/experimental/chunk/_loader.py 71.66% <0.00%> (-1.67%) ⬇️
... and 14 more

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 e1f6779...a63c58f. Read the comment docs.

@brisvag
Copy link
Contributor

brisvag commented Jul 25, 2022

Seems like a real fail... are the readers actually being installed?

@tlambert03
Copy link
Contributor Author

Yeah agreed, will look. They certainly make it into the local sdist/wheel.

Copy link
Contributor

@sofroniewn sofroniewn left a comment

Choose a reason for hiding this comment

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

Love this so much! @tlambert03 if you can chase down that test fail then I think this can merge!! 🚀

@tlambert03
Copy link
Contributor Author

that seems to have fixed it, just split up the --pyargs tests into two steps

@sofroniewn sofroniewn requested a review from jni July 25, 2022 23:23
@sofroniewn
Copy link
Contributor

Nice - all tests now passing. Let's see if @jni has any comments on this - otherwise i think it's good to merge!

@sofroniewn
Copy link
Contributor

This has been approved for a week now - unless hearing anything else by tomorrow I will merge, thanks all!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qt Relates to qt task Tasks for contributors and maintainers tests Something related to our tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants