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

NPE1Adapter Part 1 - updated _from_npe1 conversion logic to prepare for locally defined objects #124

Merged
merged 6 commits into from
Mar 23, 2022

Conversation

tlambert03
Copy link
Collaborator

@tlambert03 tlambert03 commented Mar 13, 2022

Breaking up #86 into three steps. This is part 1

  • A lot of changes have been made to the_from_npe1 module itself to prepare for the shim logic of being able to recover a locally defined objects.
  • A more realistic npe1-plugin, that contains a number of tricky scenarios with locally defined widgets and sample data generators.
  • The general idea is that if an npe1 plugin creates a locally defined object within the scope of a hook implementation, then we store a shim "python_name" in the form __npe1shim__.hook_module::hoo_impl_shimidx. For example: __npe1shim__.npe1_module:napari_experimental_provide_dock_widget_2 means "call the napari_experimental_provide_dock_widget function in the npe1_module and retrieve the 2nd object in the list
  • dependence on napari-plugin-engine has been completely removed, since all we really needed was the iter_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.

@codecov
Copy link

codecov bot commented Mar 13, 2022

Codecov Report

Merging #124 (4d27552) into main (41c557d) will decrease coverage by 1.89%.
The diff coverage is 86.33%.

@@             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     
Impacted Files Coverage Δ
npe2/manifest/utils.py 87.40% <75.38%> (-12.60%) ⬇️
npe2/_from_npe1.py 95.78% <92.37%> (-4.22%) ⬇️
npe2/__init__.py 100.00% <0.00%> (ø)
npe2/_plugin_manager.py 100.00% <0.00%> (ø)
npe2/manifest/schema.py 100.00% <0.00%> (ø)
npe2/_command_registry.py 100.00% <0.00%> (ø)
npe2/_dynamic_plugin.py 100.00% <0.00%> (ø)

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 41c557d...4d27552. Read the comment docs.

@tlambert03 tlambert03 requested a review from nclack March 14, 2022 13:32
Copy link
Collaborator

@nclack nclack left a 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.

tests/conftest.py Show resolved Hide resolved
npe2/_from_npe1.py Show resolved Hide resolved
@tlambert03
Copy link
Collaborator Author

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]
Copy link
Collaborator

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.

@Carreau
Copy link
Collaborator

Carreau commented Mar 23, 2022

+1, I can't merge because of codecov, should I deactivate it in setting for now ? It is likely due for github issues yesterday.

@Carreau
Copy link
Collaborator

Carreau commented Mar 23, 2022

pushed an empty commit trying to kick CI to get coverage and be able to merge

@tlambert03 tlambert03 merged commit cb45d7d into napari:main Mar 23, 2022
@tlambert03 tlambert03 deleted the shim1-conversion branch March 23, 2022 16:57
@tlambert03 tlambert03 changed the title NPE1Shim Part 1 - updated _from_npe1 conversion logic to prepare for locally defined objects NPE1Adapter Part 1 - updated _from_npe1 conversion logic to prepare for locally defined objects Mar 23, 2022
@tlambert03 tlambert03 added the enhancement New feature or request label Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants