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

Turn menus contributions into dict of arbitrary key to list of MenuItems #175

Merged
merged 7 commits into from
Jun 13, 2022

Conversation

tlambert03
Copy link
Collaborator

@tlambert03 tlambert03 commented Jun 13, 2022

this includes #174

@sofroniewn toyed with this pattern at some point in the hackathon... I think this is the right way for us to do menus.

Menus are still a key value pair, where the key is a string "menu id" (could be anything, not validated at the moment of manifest consumption) and the value is a List of MenuItem, where MenuItem is the union of a SubMenu or MenuCommand.

I dug into vscode a bit more, and this is how they do it. There is no validation of the key at ingestion of the manifest. You can put nonsense and it fails quietly. Napari then asks for all of the contributions of a given menu key. I don't think that "quiet" failure is a problem, particularly given that we don't know what the set of all valid keys is at that moment. If anyone is concerned about that, I think:

  1. If we wanted to, we could provide a flag in npe2 validate that warns if a key is not a known napari key (i.e., if a plugin wants to opt in to only ever contributing to napari keys, we can tell them if they've done anything wrong... but as a command line tool)
  2. I think failing quietly is fine in this case, because a plugin developer is going to try to add something to a menu, and if it doesn't appear, they'll go back and check it. If you try to add something with a typo in vscode manifest, nothing shows up... and it's pretty immediately obvious during development that you made a mistake

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #175 (7b4440f) into main (1f253d8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #175   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           27        27           
  Lines         1797      1789    -8     
=========================================
- Hits          1797      1789    -8     
Impacted Files Coverage Δ
npe2/_plugin_manager.py 100.00% <100.00%> (ø)
npe2/manifest/contributions/__init__.py 100.00% <100.00%> (ø)
npe2/manifest/contributions/_contributions.py 100.00% <100.00%> (ø)
npe2/manifest/contributions/_menus.py 100.00% <100.00%> (ø)
npe2/plugin_manager.py 100.00% <100.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 1f253d8...7b4440f. Read the comment docs.

@nclack
Copy link
Collaborator

nclack commented Jun 13, 2022

There is no validation of the key at ingestion of the manifest. You can put nonsense and it fails quietly.

I think failing quietly is good. I'd like to have structural validation - that the entry in the manifest is well-formed. But the missing foreign reference can fail quietly.

Warning on npe2 validate sounds good.

@tlambert03
Copy link
Collaborator Author

I'd like to have structural validation - that the entry in the manifest is well-formed.

Yeah, all the MenuItems will still receive standard validation. (i.e. it still needs to be a List[MenuItem]), but the menu key itself is just any string

@tlambert03 tlambert03 merged commit 92c86ea into napari:main Jun 13, 2022
@tlambert03 tlambert03 deleted the new-menus branch June 13, 2022 15:03
@tlambert03 tlambert03 added the enhancement New feature or request label Jun 13, 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.

2 participants