napari / docs

Documentation for napari (other than API docs)
BSD 3-Clause "New" or "Revised" License
11 stars 36 forks source link

Plugin development docs seemingly copy-paste-edit includes type aliases that must be defined (or imported?) #365

Closed vreuter closed 6 months ago

vreuter commented 7 months ago

📚 Documentation

The plugin contribution guide's reader example includes a code snippet that's made available for copy-and-paste and makes a nice template that could be edited for quick plugin development, but it includes the LayerData type alias that's note explained on the same page, and isn't provided in code.

Following the link on the same page (reader contribution guide) to the page for layer data tuples, we see the type alias explained in the formal type definition. That definition, in turn, contains other type aliases which, while detailed in the code below it on that same page, includes a reference to a Protocol subtype (ArrayLike) that's noted as an approximation rather than a real intended definition.

How is a developer of a reader plugin, then, to type the return value of a get_reader function? It should be Optional[ReaderFunction], but following the path of contained types (ReaderFunction --> LayerData --> DataType --> ArrayLike) leads somewhere to a type (ArrayLike) which isn't clear to what this name concretely refers.

Are these type aliases available somewhere for import? If not, how to proceed with the type signature for a reader plugin?

vreuter commented 7 months ago

Possible, I suppose, to reference something like this? https://numpy.org/devdocs/reference/typing.html#numpy.typing.ArrayLike

melissawm commented 7 months ago

Thanks for the report @vreuter - I am moving this issue to the napari/docs repo as our documentation pages live there. Feel free to engage there. Cheers!

psobolewskiPhD commented 7 months ago

@DragaDoncila @lucyleeow Would appreciate your thoughts on this one! ❤️

vreuter commented 7 months ago

Thanks for the report @vreuter - I am moving this issue to the napari/docs repo as our documentation pages live there. Feel free to engage there. Cheers!

Sorry for misplacing initially, I'm pretty new to napari 😅 ! Thanks for migrating it @melissawm

lucyleeow commented 7 months ago

AFAICT there is no need to annotate the get_reader or the file_reader functions. E.g., in the cookiecutter -> reader example, neither are annotated.

I think the annotations are just there in the docs to try and provide info, but I see it has caused confusion. Not sure what the best solution is. Maybe a note saying the annotations are just there for info, you do not need to annotate anything. If we don't feel the annotation in the example is giving useful info we could remove it (but I do feel like its helpful).

Note we should be aware when writing docs that annotations are not always unnecessary, there is one case (that I know of) where annotation (and correct annotation) is important -> when using magicgui function widgets, see: https://napari.org/dev/howtos/extending/magicgui.html#return-annotations

vreuter commented 7 months ago

Thanks for the info @lucyleeow

If we don't feel the annotation in the example is giving useful info we could remove it (but I do feel like its helpful)

I totally agree, it's very useful! Helped me greatly in understanding the structure of the values I needed to work with to write a plugin. Definitely in favor of keeping it. I was momentarily frustrated by difficulty in replicating the cleanness of the types in the documentation in my own code 😅

I propose #367 , WDYT?

vreuter commented 7 months ago

Definitely in favor of keeping it.

Since it's a code snippet that's not meant to be immediately functional, I think it'd be nice if the copy icon for copy/paste were removed, but I don't know how this the docs are rendered / if this is possible (?), or even if there'd be agreement, so I left that potential change off of the PR, and it's less important I think anyhow.

psobolewskiPhD commented 7 months ago

The copy button is generated by sphinx_copybutton. Here's the docs: https://sphinx-copybutton.readthedocs.io/en/latest/use.html

Seems like you'd need a css class to make it optional.

lucyleeow commented 7 months ago

Good point about the copy-paste code snippet. What if we made the annotations strings? I think that would still give functioning code right?

Also I think we should add a note in the 'Reader' section explaining the annotation is for information only but not required, which is why they are strings.

Czaki commented 7 months ago

AS I know, it is possible to add additional text to copied one in JS. Maybe hide import from rendering but add them is someone copies code snippet?

lucyleeow commented 7 months ago

If we were to add the imports, I think it is fine to have them render. It would be 'unexpected behaviour' to have what you copy to be different from what you see.

If we do add the imports, I think we should put them under if TYPE_CHECKING as otherwise we would add unnecessary imports to the plugin dev's code.

I think using strings would also be an acceptable option.