-
Notifications
You must be signed in to change notification settings - Fork 28
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
NPE1Adapter Part 1 - updated _from_npe1 conversion logic to prepare for locally defined objects #124
Conversation
Codecov Report
@@ Coverage Diff @@
## main #124 +/- ##
===========================================
- Coverage 100.00% 98.10% -1.90%
===========================================
Files 23 24 +1
Lines 1355 1581 +226
===========================================
+ Hits 1355 1551 +196
- Misses 0 30 +30
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. It was very helpful talking about this earlier. I made some notes where I wasn't seeing test coverage, but I think the later pr's get those. Those notes are mostly for me. I should get to the other pr's later tonight.
Yeah correct. Mentioned this in the original post, but this one can merge without 100% coverage, tests are in part 2 |
module_name, funcname = match.groups() | ||
_validators.python_name(python_name) # shows the best error message | ||
match = _validators.PYTHON_NAME_PATTERN.match(python_name) | ||
module_name, funcname = match.groups() # type: ignore [union-attr] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just split on :
? I guess using regex to validate even more sure, but if it's invalid it will anyway fail just after.
+1, I can't merge because of codecov, should I deactivate it in setting for now ? It is likely due for github issues yesterday. |
pushed an empty commit trying to kick CI to get coverage and be able to merge |
Breaking up #86 into three steps. This is part 1
_from_npe1
module itself to prepare for the shim logic of being able to recover a locally defined objects.npe1-plugin
, that contains a number of tricky scenarios with locally defined widgets and sample data generators.__npe1shim__.hook_module::hoo_impl_shimidx
. For example:__npe1shim__.npe1_module:napari_experimental_provide_dock_widget_2
means "call thenapari_experimental_provide_dock_widget
function in thenpe1_module
and retrieve the 2nd object in the listnapari-plugin-engine
has been completely removed, since all we really needed was theiter_hookimpls
logic which has been recreated here.note: this one can merge without full test coverage
steps 2 and 3 provide the 100% test coverage, but this one doesn't, as it really needs logic from those later PRs to cover it.