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

Inlcude docstring of make_napari_viewer in testing.md #378

Merged
merged 17 commits into from
Mar 27, 2024

Conversation

lucyleeow
Copy link
Collaborator

@lucyleeow lucyleeow commented Mar 26, 2024

References and relevant issues

Towards #150
Depends on napari/napari#6774

Description

Use autofunction to include docstring of make_napari_viewer

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Mar 26, 2024
@lucyleeow lucyleeow changed the title try autofunction Inlcude docstring of make_napari_viewer in testing.md Mar 26, 2024
@lucyleeow lucyleeow added this to the 0.5.0 milestone Mar 26, 2024
@lucyleeow
Copy link
Collaborator Author

I think this is ready for review now.
I tried to not include the full module path to make_napari_viewer but I couldn't figure out how to do this in just this instance. It's probably fine that the full path is rendered:

image

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

I have a few small nitpicks, but otherwise I like it.

Comment on lines 204 to 206
> note: fixtures in pytest can be a little mysterious, since it's not always
> clear where they are coming from. In this case, using a pytest-qt fixture
> looks like this:
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this a proper admonition? It kinda looks like a code block.
I think this is actually pretty important, especially when learning, so a proper admonition would make it stand out more.

Copy link
Member

Choose a reason for hiding this comment

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

and in the admonition i'd put the python module name pytest-qt in backticks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do and I've added that pytest-qt is a napari testing dependency.

docs/developers/contributing/testing.md Outdated Show resolved Hide resolved
docs/developers/contributing/testing.md Outdated Show resolved Hide resolved
@Czaki
Copy link
Contributor

Czaki commented Mar 26, 2024

I think that we should more promote make_napari_viewer_proxy as it will provide wrapped viewer, same as will be passed to plugins widgets.

@lucyleeow
Copy link
Collaborator Author

Thanks @psobolewskiPhD , changes made!

@Czaki so this testing section is aimed at napari developers, there is a separate section on testing for plugin developers. Are you recommending make_napari_viewer_proxy to be used for plugin testing or napari testing?

I will make another PR to add cross references to other testing sections.

Copy link
Member

@psobolewskiPhD psobolewskiPhD left a comment

Choose a reason for hiding this comment

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

thanks for your patience!

@lucyleeow
Copy link
Collaborator Author

No thank you for your patience!

I just added a empty line at the end of the note because I thought the code block looked weird otherwise. Good to go if you're happy.

image

Copy link
Contributor

@DragaDoncila DragaDoncila left a comment

Choose a reason for hiding this comment

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

This looks good to me! Since this is for internal tests I don't think we should change this to use the viewer proxy.

@psobolewskiPhD
Copy link
Member

I think the spacing/padding/margin -- if I am understanding correctly -- is fixed in the theme:
napari/napari-sphinx-theme#100
But probably not released.

@psobolewskiPhD psobolewskiPhD merged commit 3ad59cb into napari:main Mar 27, 2024
6 checks passed
@lucyleeow lucyleeow deleted the doc_mnv branch March 27, 2024 22:52
@lucyleeow
Copy link
Collaborator Author

I think the spacing/padding/margin -- if I am understanding correctly -- is fixed in the theme:

@psobolewskiPhD in that case I am happy to remove the empty line? I did notice there are other places with similar situation, so we may as well be consistent with what is happening already.

@psobolewskiPhD
Copy link
Member

❤️
Let's wait for the release and then re-assess.

psobolewskiPhD added a commit that referenced this pull request Mar 31, 2024
# References and relevant issues
Follow on from #378
Depends on #325

# Description
Add cross reference to other testing info in `testing.md` (waiting on
#325 so I can add cross ref label)

---------

Co-authored-by: Peter Sobolewski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants