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

npe2 docs #3769

Closed
wants to merge 35 commits into from
Closed

npe2 docs #3769

wants to merge 35 commits into from

Conversation

nclack
Copy link
Contributor

@nclack nclack commented Dec 8, 2021

NOTE

This PR has had a lot of comments and I've dirtied it up a bit. I'm going to clean it up and open a new PR for final review.

Description

This adds documentation for npe2 to the napari docs. Some of the details are awaiting the last few changes to the npe2 manifest before release.

I'll keep this as draft until npe2 release. Please feel free to review while this is in draft.

There is a

  • npe2 manifest specification
  • npe2 getting started guide
  • npe2 migration guide
  • introduction of npe2 on the plugin index page
  • added new pages to the top level _toc.yml

Type of change

  • This change requires a documentation update

Todo

docs/plugins/index.md Outdated Show resolved Hide resolved
@nclack nclack marked this pull request as draft December 8, 2021 05:44
@nclack
Copy link
Contributor Author

nclack commented Dec 8, 2021

@tlambert03 @DragaDoncila @Carreau

I'd love feedback!

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #3769 (5aabfa6) into main (d054fdb) will decrease coverage by 0.10%.
The diff coverage is 84.62%.

❗ Current head 5aabfa6 differs from pull request most recent head 1893059. Consider uploading reports for the commit 1893059 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3769      +/-   ##
==========================================
- Coverage   83.05%   82.95%   -0.11%     
==========================================
  Files         563      576      +13     
  Lines       46524    47451     +927     
==========================================
+ Hits        38642    39362     +720     
- Misses       7882     8089     +207     
Impacted Files Coverage Δ
napari/_qt/_tests/test_app.py 60.60% <ø> (-2.26%) ⬇️
napari/_qt/layer_controls/qt_points_controls.py 89.26% <0.00%> (-1.22%) ⬇️
napari/_qt/layer_controls/qt_shapes_controls.py 91.94% <0.00%> (-1.26%) ⬇️
napari/_qt/menus/debug_menu.py 71.42% <0.00%> (ø)
napari/_qt/menus/help_menu.py 100.00% <ø> (ø)
napari/_qt/qt_event_loop.py 70.14% <0.00%> (-0.53%) ⬇️
napari/_tests/test_adding_removing.py 100.00% <ø> (ø)
napari/_tests/test_draw.py 36.84% <0.00%> (ø)
...ispy/experimental/_tests/test_vispy_tiled_image.py 27.27% <0.00%> (ø)
napari/benchmarks/benchmark_qt_viewer_image.py 0.00% <0.00%> (ø)
... and 163 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 fcde732...1893059. Read the comment docs.

@nclack nclack mentioned this pull request Dec 8, 2021
17 tasks

```bash
pip install -e .
napari
Copy link
Member

Choose a reason for hiding this comment

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

Can we update this to be a single command that "executes" the plugin? e.g. napari path/to/file.npy? That's not a super interesting example, but I always have to look up the equivalent command for pre-loading a plugin widget.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed it would be nice to bring that convenience from npe1 over. We're still thinking about how to accomplish it

Copy link
Member

@tlambert03 tlambert03 left a comment

Choose a reason for hiding this comment

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

not a full review yet... but don't want to dump it all later :)

docs/plugins/index.md Outdated Show resolved Hide resolved
docs/plugins/index.md Outdated Show resolved Hide resolved
docs/plugins/index.md Outdated Show resolved Hide resolved
docs/plugins/index.md Outdated Show resolved Hide resolved

## How to build plugins?

For a guide on how to create a plugin, see
Copy link
Member

Choose a reason for hiding this comment

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

before this page gets published, i think it should be super clear, on every "how to page" whether it's referring to npe1 or npe2. i don't see us referring to "hook specifications" quite as much with npe2, but rather "contributions"... so this index page, guiding people into what they might want to look at next, needs to be super clear about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. On the index page, I'm not quite sure how to deal with the npe1 to npe2 transition. I'll add text here making it clear this references npe1 and I'll add some text re npe2. I'll also be adding admonitions to the tops of the other plugin pages noting that npe2 exists and the current page deals with X plugin engine.

"hook specifications" ... "contributions"

I'm definitely trying to follow this convention throughout. "contributions" for npe2 and "hook specs" or "hook impls" for npe1.

`my_npy_reader`. This is the name that will be used for the python package. It
should conform to the [PEP8][] naming convention.

When the cookiecutter asked to include a reader plugin, we selected "y", and
Copy link
Member

Choose a reason for hiding this comment

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

(When the cookiecutter asked to include a reader plugin, could be removed with line numbers)

docs/plugins/npe2_getting_started.md Outdated Show resolved Hide resolved
docs/plugins/npe2_getting_started.md Outdated Show resolved Hide resolved
docs/plugins/npe2_getting_started.md Outdated Show resolved Hide resolved

```bash
pip install -e .
napari
Copy link
Member

Choose a reason for hiding this comment

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

Agreed it would be nice to bring that convenience from npe1 over. We're still thinking about how to accomplish it

nclack and others added 4 commits December 12, 2021 15:32
Co-authored-by: Talley Lambert <talley.lambert@gmail.com>
Co-authored-by: Talley Lambert <talley.lambert@gmail.com>
Co-authored-by: Talley Lambert <talley.lambert@gmail.com>
chili-chiu and others added 4 commits December 13, 2021 15:21
* introduce event debugging

* update sample

* one more comment

* exclude .env_sample from check-manifest

* allow toggle

* more docs
@github-actions github-actions bot added the qt Relates to qt label Dec 13, 2021
@nclack nclack marked this pull request as ready for review December 14, 2021 07:30

(commands)=

## Commands
Copy link
Member

Choose a reason for hiding this comment

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

I'd really like to see almost all of this file be pulled from the Field(description=...) and the class docstrings of the npe2 manifest. Otherwise, we'll always have an issue with knowing whether they're in sync. If you want some tips for that, let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love tips!


(top-level-props)=

## Top-level properties
Copy link
Member

Choose a reason for hiding this comment

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

I think Contributions should have its own page, rather than each individual subsection of contributions appearing as a flat header next to "top level props"

Copy link
Member

Choose a reason for hiding this comment

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

I think this page is extremely easy to navigate: https://code.visualstudio.com/api/references/contribution-points

might just ignore the `point` layers. The writer must return `None` for
unwritten layers.

### Calling convention
Copy link
Member

Choose a reason for hiding this comment

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

we use the term function signature most other places. any reason not to keep that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use "calling convention" to refer to both the function signature and the requirements around when it's called.

For example, single and multi layer writers have different function signatures. Their behavior relative to one another is described by a calling convention.

Another example is the "autogenerate" flag modifying function widgets. The calling convention describes that, in the context where the function is invoked, it will be wrapped to produce a widget.

@@ -0,0 +1,606 @@
(npe2-manifest-spec)=
Copy link
Member

Choose a reason for hiding this comment

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

general thought on this file. I feel like it reads somewhere in between "tutorial" and "reference" ... making it a bit less useful for both purposes. If someone's made a plugin before and just can't remember what the manifest specification is, there should be a page where they can go to essentially see the schema, with bullet points for the fields, and probably one example. We also certainly need all of the more tutorial-like content here, but having them all together makes it a bit more cumbersome to reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree - I like the idea of separating tutorial/concepts/reference. I also want to add guides for the specific plugin types.

I don't want to have to address that in this PR though. It'll be too complicated. I'd like to get this in as a good starting place, then work out a set of issues so we can get the structure where we want it piece by piece.

`npe2` as soon as possible. See the [](npe2-migration-guide).
```

The **plugin manifest** is a specially formatted text file declaring the
Copy link
Member

Choose a reason for hiding this comment

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

we've already said this elsewhere (in the entry page). I'd love to see our documentation be leaner, which means we don't really need someone to be able to enter anywhere and bring them back up to speed. Let the top pages give the background, the inner pages be the reference, and the tutorial pages read like hand-holding how-to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I might have removed this from the plugin index page. I have it here because this is where I define all these terms.

defining the fields and their data types. These fields and their meanings are
described below.

A **plugin engine** is used to discover plugins, provide utilities for
Copy link
Member

Choose a reason for hiding this comment

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

they don't need to know this when writing the manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do refer to the "plugin engine" later on. But I don't refer to discovery. Should I remove the paragraph? It looks like a pretty reasonable idea. I could move the definition of plugin engine down to where it's used.


(compatibility)=

### Compatibility
Copy link
Member

Choose a reason for hiding this comment

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

there's a lot of background here, which is perhaps of interest to napari developers looking to modify the schema, but a lot for someone just writing a plugin who more or less just needs to know which number to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree this is much too long. Can you suggest how you'd like to shrink it down?

I kind of like keeping just the first paragraph. Probably with a different section title.

@github-actions github-actions bot removed the qt Relates to qt label Dec 14, 2021
@nclack nclack marked this pull request as draft December 14, 2021 21:51
nclack added a commit to nclack/napari that referenced this pull request Dec 14, 2021
@nclack nclack mentioned this pull request Dec 14, 2021
14 tasks
@nclack nclack closed this Dec 14, 2021
@nclack
Copy link
Contributor Author

nclack commented Dec 14, 2021

closing in favor of #3818. This pr has gotten hard to review so I cleaned it up and made a new pr.

sofroniewn pushed a commit that referenced this pull request Dec 15, 2021
* npe2 docs (ref #3769)

* Update docs/plugins/npe2_manifest_specification.md

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* review update

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
@nclack nclack deleted the nclack/docs-npe2 branch December 15, 2021 16:08
jni pushed a commit that referenced this pull request Dec 16, 2021
* npe2 docs (ref #3769)

* Update docs/plugins/npe2_manifest_specification.md

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>

* review update

Co-authored-by: Andy Sweet <andrew.d.sweet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants