refactor underlying architecture of plugin for molularity and encapsulation #67
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 11, 2023
alessandrofelder
force-pushed
the
refactor-class-structure
branch
3 times, most recently
from
5daa20a
to
4e35311
Compare
alessandrofelder
force-pushed
the
refactor-class-structure
branch
from
4e35311
to
844b239
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 91.85% 97.32% +5.47%
==========================================
Files 6 7 +1
Lines 270 299 +29
==========================================
+ Hits 248 291 +43
+ Misses 22 8 -14
☔ View full report in Codecov by Sentry. |
|
I am going to fix the slight decrease in coverage, but in the meantime it would be great if, @adamltyson, you could check that you agree that nothing is broken for the user by this PR (it's supposed to be a pure moving-stuff-around and communicate-via-qt-signals-instead PR) with no changes in functionality for the user? |
* signal was previously not emitted properly in table view's qmenu * rename test atlas_viewer_widget > brainrender_widget * add tests for model header and additional reference context menu
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
What is this PR
Why is this PR needed?
The
atlas_viewer_widget.pyfile was becoming unwieldy to add functionality to, and things that were napari-independent were tested in a napari context as a consequence of the previous architecture (which made the tests slow, as well as violating separation of concerns).What does this PR do?
This PR splits the classes more independent and have clearer responsibilities. Some things have been renamed for clarity too.
The plugin should look and behave exactly the same as before (bar also fixing #66 )
a key advantage is better modularity
* widgets can be used and tested independent of napari (and the tests seem faster to run?)
* each file and class has clear responsibilities
* easier to add new features to because the amount of context needed to worry about at once is reduced
a downside is that there is an extra layer of abstraction in the code(but I think it's worth it!)
References
Closes #8
Closes #66
How has this PR been tested?
Tests have been refactored accordingly, and pass. Also extensive manual testing for any unintentional changes in behaviour.
Is this a breaking change?
Not for the user, big change in API which is good to be done early in the development process for this plugin
Does this PR require an update to the documentation?
Will be added as part of #52. Docstrings improved and added as needed by the refactoring.
Checklist: