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

Adding documents and images from January 2022 plugin testing workshop. #35

Merged
merged 82 commits into from
Feb 14, 2023

Conversation

Nadalyn-CZI
Copy link
Contributor

@Nadalyn-CZI Nadalyn-CZI commented Oct 27, 2022

Description

The first section of the workshop has been broken down into 5 articles. They are: 1. Python's assert keyword, 2. Pytest testing frameworks, 3. Napari plugins, and 4. Testing coverage. The 5th one on testing widgets is on hold for now. Feedback is encouraged.

Type of change

  • This change adds new documentation

Final checklist:

  • My PR is the minimum possible work for the desired functionality
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • If I included new strings, I have used trans. to make them localizable.
    For more information see our translations guide.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 27, 2022
@melissawm melissawm added content Ideas for new or improved content and removed documentation Improvements or additions to documentation labels Oct 27, 2022
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Oct 27, 2022
@Nadalyn-CZI
Copy link
Contributor Author

These documents should be reviewed by Draga Pop, Chi-Li Chiu, seankmartin, and the members of the Metacell group.

@Nadalyn-CZI Nadalyn-CZI marked this pull request as ready for review November 3, 2022 19:27
@chili-chiu
Copy link
Contributor

@psobolewskiPhD would love your feedback here too!

Copy link
Contributor

@seankmartin seankmartin left a comment

Choose a reason for hiding this comment

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

Hi @Nadalyn-CZI, thank you so much for the amazingly detailed documentation about this workshop! 🚀

There are a few little nitty formatting things that could be improved upon. I have left a few comments and suggestions that might give an idea of these changes. @chili-chiu the changes I have listed below in the review are not complete, they are just to give an idea. I'm happy to make a follow up commit to this PR adjusting formatting in this style if you would like!

As for the overall content, it looks great in general. I might have some further feedback if the formatting is adjusted as I will be able to read the built documentation a bit easier at that point.

docs/_toc.yml Outdated Show resolved Hide resolved
docs/plugins/testing_workshop_docs/Test-coverage.md Outdated Show resolved Hide resolved
docs/plugins/testing_workshop_docs/Test-coverage.md Outdated Show resolved Hide resolved
@Nadalyn-CZI
Copy link
Contributor Author

Wow! Sean! Thank you so much.

@Nadalyn-CZI
Copy link
Contributor Author

Nadalyn-CZI commented Nov 14, 2022 via email

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.

Love this!
I have a number of small comments.
Also, I worry that this can be silo'd in the workshops and maybe should be surfaced to How-to/Tutorials? (I'm not clear what goes where). Maybe @melissawm has some thoughts?

docs/plugins/testing_workshop_docs/Test-coverage.md Outdated Show resolved Hide resolved
docs/plugins/testing_workshop_docs/Test-coverage.md Outdated Show resolved Hide resolved
docs/plugins/testing_workshop_docs/Test-coverage.md Outdated Show resolved Hide resolved
docs/plugins/testing_workshop_docs/Testing-Resources.md Outdated Show resolved Hide resolved
docs/plugins/testing_workshop_docs/Testing-Resources.md Outdated Show resolved Hide resolved
Nadalyn-CZI and others added 6 commits January 22, 2023 14:48
Clarify wording and correct typos.

Explanation about testing widgets.

Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: Sean Martin <martins7@tcd.ie>
Co-authored-by: Sean Martin <martins7@tcd.ie>
Co-authored-by: Draga Doncila Pop <17995243+DragaDoncila@users.noreply.github.com>
Co-authored-by: melissawm <melissawm@gmail.com>
@melissawm
Copy link
Member

melissawm commented Jan 22, 2023

I think we're almost there, there are still a few things I noticed:

  • We should probably crop both docs/images/coverage_report.png and docs/images/second_coverage_report.png to remove windows decorations
  • There is quite a lot of whitespace at the end of lines. Personally I tend to remove it all (and it can be done automatically) but this is a matter of style, it won't cause issues.
  • I have changed all curly braces in code to be straight (" instead of “). They are very similar but like @seankmartin mentioned somewhere above, they are not parsed correctly by Markdown so they are best avoided, unless they come from raw output copy-paste (for example, in the documents above there are a few console output sessions that have curly braces. Those are ok.)
  • I have also changed a few "bash" blocks to "console" - I found that they were not rendering correctly before but they are now.
  • Finally, there is the questions of the "Article" naming. Like I said I don't have a strong opinion but it may be worth deciding on a pattern.

@Nadalyn-CZI I have also removed the Activity Tab folder (I think you meant to add those to another pull request and they came up here by accident)

Hope this is better, the docs are now building cleanly and you can see the artifacts above. Cheers!

EDIT: @Nadalyn-CZI to update your local copy of this pr, you will need to checkout the testing_workshop branch and pull the latest changes that I made. This can be done via the GitHub Desktop app or on the console by doing the following: if your fork is called origin (please confirm by doing git remote -v), then:

git pull origin testing_workshop --rebase

You may be presented with conflicts you will have to resolve, but I hope this is not too bad. Ping me if you need help figuring it out 😄

@Nadalyn-CZI
Copy link
Contributor Author

Nadalyn-CZI commented Jan 23, 2023 via email

@melissawm
Copy link
Member

It is up to you, what I'm suggesting is removing the Chrome menu bar at least - the purple one at the top of the images. If you prefer keeping the address that is fine, I think. I will defer to the maintainers on this one though 😄

@Nadalyn-CZI
Copy link
Contributor Author

@DragaDoncila and @psobolewskiPhD:

Once again, I think this is ready to merge. Would you please review and merge it if there are no problems with it?

Thanks,

Nadalyn

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.

Approving now (just removing the last few mentions of the word article). @melissawm would you mind doing a final pass, as I've noticed you still have changes requested 😊

Copy link
Member

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

Last comments from me, but after those this is good to go! Thanks @Nadalyn-CZI !

Clarifications from Melissa.

Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
@Nadalyn-CZI
Copy link
Contributor Author

Nadalyn-CZI commented Feb 9, 2023 via email

@melissawm
Copy link
Member

Now, we wait for a merge (unless there are any further observations!) 😄

@DragaDoncila
Copy link
Contributor

@Nadalyn-CZI woohoo this is looking great 🎉 ! With @melissawm's approval I'm now starting the 24 hour merge clock on this and will merge tomorrow morning or first thing Monday 😊

@psobolewskiPhD
Copy link
Member

Don't want to step on any toes, but I'm going to merge this! It's gotten plenty of time I think.
Thanks everyone involved! 🙏 ❤️

@psobolewskiPhD psobolewskiPhD merged commit f0142b5 into napari:main Feb 14, 2023
@Nadalyn-CZI
Copy link
Contributor Author

Nadalyn-CZI commented Feb 14, 2023 via email

@DragaDoncila
Copy link
Contributor

Thanks @psobolewskiPhD ! Turns out I suck at 24 hour merge clocks 😅

@psobolewskiPhD psobolewskiPhD added this to the 0.4.18 milestone Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Ideas for new or improved content documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants