search

spyder-ide / spyder-docs

Documentation for Spyder, the Scientific Python Development Environment
https://docs.spyder-ide.org
MIT License
33 stars 283 forks source link

PR: Add documentation for the Spyder-Unittest plugin #272

Closed pkpatricia closed 2 years ago

pkpatricia commented 3 years ago

Pull Request

Pull Request Checklist

Description of Changes

Issue(s) Resolved

Fixes #

juanis2112 commented 3 years ago

@jitseniesen we have now docs for our Unittest plugin thanks to @pkpatricia who worked on it during our Scipy sprints! Please let us know what you think about this PR and if there is something we are missing.

pkpatricia commented 2 years ago

added suggested changes

CAM-Gerlach commented 2 years ago

Hey @pkpatricia , thanks for all your great work on this, and it looks this is getting close to merge-worthy. Once you've addressed the outstanding feedback and you're ready for another review, please ping me and I can do a pass to check for technical, copyediting and style issues (I'm unclear whether this is ready, as you left a comment saying the suggestions were applied, but then made further commits and I don't see the other reviewers re-requested for review). Most if not all of these should be doable as suggestions and not require any additional work from you other than batch-commiting the changes. If you prefer, given they should all be very minor, I can make them as a commit directly on your branch (minus any that aren't strictly technical in nature).

Speaking of which, when a reviewer makes a number of suggestions in a single review, its usually much better to use Github's "batch" feature to add each commit to commit the changes all at once. From the Files view, you just click "add to batch" on each of the suggestions, and then when they are all selected, and then at the top, add an appropriately descriptive commit message (e.g. Apply reviewer feedback on Unittest plugin docs or Address style issues in Unittest plugin docs), and then click the Commit button. This will avoid lots of trivial commits with duplicate, uninformative commit messages that will need to be fixuped before merging, as well as running our CIs many more times and making it more work to review.

To fix the commits already made, you'll need to run git rebase -i 5f59c26fde2221d557c2c7e94a13e7f2ee2db0b0, and change the pick next to all but the first (top) commit in each batch to fixup, and then save and close the file. Git will do the rebase, and then you'll need to git push -f to push them. Alternatively, if you're not comfortable with rewriting git history (a mistake will require more git fu to undo), we can do it ourselves before merging. The fixes/optimizations to the PNG images also needs to be rebased in, which we can also do for you if you prefer.

Thanks!

CAM-Gerlach commented 2 years ago

Hey @pkpatricia , sorry to bug you and not sure if you saw my earlier comment, but is this ready for another pass? Thanks!

Also, just FYI, I marked it as draft as a reminder to us to make sure that the duplicate commits and optimized images are rebased in before irrevocably merging, per our standard procedure.

pkpatricia commented 2 years ago

I'm super sorry. I'm getting the hang of how git pull requests work. Yes, I believe this is ready for submission. When I first started this process, Carlos and Juanita instructed me on using make docs and optimizing images. Either way I know that inorder for this to be posted it must be correct so I'm open to improvement.

Best Regards "You always have two choices: your commitment versus your fear." -Sammy Davis, Jr. https://patriciaparker.co

On Mon, Sep 20, 2021 at 7:33 PM CAM Gerlach @.***> wrote:

Hey @pkpatricia https://github.com/pkpatricia , thanks for all your great work on this, and it looks this is getting close to merge-worthy. Once you've addressed the outstanding feedback and you're ready for another review, please ping me and I can do a pass to check for technical, copyediting and style issues (I'm unclear whether this is ready, as you left a comment saying the suggestions were applied, but then made further commits and I don't see the other reviewers re-requested for review). Most if not all of these should be doable as suggestions and not require any additional work from you other than batch-commiting the changes. If you prefer, given they should all be very minor, I can make them as a commit directly on your branch (minus any that aren't strictly technical in nature).

Speaking of which, when a reviewer makes a number of suggestions in a single review, its usually much better to use Github's "batch" feature to add each commit to commit the changes all at once. From the Files view, you just click "add to batch" on each of the suggestions, and then when they are all selected, and then at the top, add an appropriately descriptive commit message (e.g. Apply reviewer feedback on Unittest plugin docs or Address style issues in Unittest plugin docs), and then click the Commit button. This will avoid lots of trivial commits with duplicate, uninformative commit messages that will need to be fixuped before merging, as well as running our CIs many more times and making it more work to review.

To fix the commits already made, you'll need to run git rebase -i 5f59c26fde2221d557c2c7e94a13e7f2ee2db0b0, and change the pick next to all but the first (top) commit in each batch to fixup, and then save and close the file. Git will do the rebase, and then you'll need to git push -f to push them. Alternatively, if you're not comfortable with rewriting git history (a mistake will require more git fu to undo), we can do it ourselves before merging. The fixes/optimizations to the PNG images also needs to be rebased in, which we can also do for you if you prefer.

Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spyder-ide/spyder-docs/pull/272#issuecomment-923480954, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACTRRIMLEAMLAJ7VVMUVFP3UC7HENANCNFSM5ARNG4DQ .

pkpatricia commented 2 years ago

@CAM-Gerlach super sorry for any confusion. Yes I believe I have made suggested changes.

CAM-Gerlach commented 2 years ago

No worries at all @pkpatricia ! You're all good, and we really appreciate your great help and your patience with our feedback. For my part, I just came back from an extended absence for personal reasons, so sorry if I'm not 100% up to date myself. It wasn't too long ago (especially for me and Juanita) that we were new to all this too; I certainly remember it being kinda overwhelming and even intimidating at times, and we're always still learning new things—and at least as for my first pull requests, they certainly weren't as impressive as this. If there's anything confusing or unclear, or something we can do to make this whole process smoother, just ask—we're here to help.

In any case, I'm looking forward to reviewing your work, and will get on that now. Once its ready to merge, its no problem at all for us to take care of cleaning up the commits and rebasing in the scaled/optimized images and GIFs (in fact, its pretty typical). Thanks again!

CAM-Gerlach commented 2 years ago

Hey @pkpatricia , I'm a little confused as to what happened...is something wrong? All this PR needed to be ready to go was batch-applying the requested copyediting, technical and style suggestions, addressing a couple previous reviewer comments and potentially making some changes to the screenshots, if the other reviewers agree and that's not too much work (otherwise, we can take care of that later). I again apologize if my technical/style/copyediting pass was rather delayed; I was unfortunately taking a break from my volunteer role with Spyder due to the passing of several people close to me. We're here to help get this across the finish line for you, and if you feel differently about any of the suggested changes, please let us know and we'll work something out. Hoping to hear from you soon.