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

Improve & add components section to Editor intro #322

Closed hyounes4560 closed 2 years ago

hyounes4560 commented 2 years ago

Pull Request

Pull Request Checklist

Description of Changes

Updates the editor introduction section

Rendered Preview

CAM-Gerlach commented 2 years ago

Hey @hyounes4560 ; thanks for your work so far and looking like a pretty good start! I particularly like the screenshots with the annotations and numbered sections!

I see I was tagged for review, but this PR is also still marked as a draft. Just to confirm, would you like me to review it now? If so, is there anything particular you'd like to be in or out of scope for the first round of review?

When it comes to things like reST syntax, Sphinx roles, source formatting, and other technical and style guide details, there's a few ways we can handle that, so let me know which you prefer (and we can always change it later on future PRs depending on what works best for you):

In the past, working with our previous writer, I initially did review suggestions, but we subsequently found directly pushing a commit taking care of the small nitpicky stuff to be a lot more efficient for both of us and did that from then on, so that might be the best option, but its ultimately up to you what you want me to do. Please let me know!

It looks like you did a great job overall following one sentence per line, but there was just one line missing a couple line breaks in item 6. Also, I noticed that (probably due to Google Docs) the standard programming quotes were replaced with typographical "smart" quotes; we normally have a pre-commit hook that autofixes them but I had to disable it temporarily due to an issue. It would make the review comments a lot easier to follow if those little issues were cleaned up first; I can take care of it for you in a commit, or you can do it if you prefer—just let me know.

Thanks!

hyounes4560 commented 2 years ago

@CAM-Gerlach thanks for your prompt response and helpful info. Yes, this PR is still a Draft as I am planning to revisit and expand other sections of the Editor's docs. It would make more sense if you guys did the review incrementally as I commit. However, it might be wiser to finish everything and mark the PR as ready. For now, you don't need to do anything. Just for your info, I had a hard time pushing my commits since Thursday due to Pre-commits and had to bypass that using --no-verify. Thanks again.

CAM-Gerlach commented 2 years ago

Yes, this PR is still a Draft as I am planning to revisit and expand other sections of the Editor's docs.

You could do the whole Editor docs in one PR, but especially with the size of the Editor pane, and the fact that its usually considered a good idea to keep PRs relatively focused on one specific thing, it might be a better idea to do it incrementally section by section, which each section corresponding to one PR (particularly given you've already aptly titled this one "Update the Editor introduction section", which I presumed was going to be the scope of the PR). This keeps the PRs more manageable, easier for us to review and easier for you to read the feedback, your changes get merged and go live quicker, and you can apply the feedback from one more easily to the next.

When you get a chance, could you let me know how you would like to approach

When it comes to things like reST syntax, Sphinx roles, source formatting, and other technical and style guide details, there's a few ways we can handle that, so let me know which you prefer

and

It would make the review comments a lot easier to follow if those little issues were cleaned up first; I can take care of it for you in a commit, or you can do it if you prefer—just let me know.

Thanks!

Just for your info, I had a hard time pushing my commits since Thursday due to Pre-commits and had to bypass that using --no-verify.

Thanks for letting me know, and sorry to hear that! Since pre-commit is passing just fine on this PR as well as on my machine, it seems there may be either an issue with your local setup, or a platform, etc. compatibility issue with one of the hooks. Either way it is something we need to fix, so could you open a new issue with some basic information about the problem so I can determine what is going on and either advise you how to fix it, or disable/revert the offending hook(s)? Specifically, could you please provide the following info:

Thanks!

steff456 commented 2 years ago

You could do the whole Editor docs in one PR, but especially with the size of the Editor pane, and the fact that its usually considered a good idea to keep PRs relatively focused on one specific thing, it might be a better idea to do it incrementally section by section, which each section corresponding to one PR (particularly given you've already aptly titled this one "Update the Editor introduction section", which I presumed was going to be the scope of the PR). This keeps the PRs more manageable, easier for us to review and easier for you to read the feedback, your changes get merged and go live quicker, and you can apply the feedback from one more easily to the next.

I think I also agree with this, so maybe what we can do is just open a PR for each section of the Editor docs. You will need to create a new branch from this PR to continue working on the following sections and then rebase when this one is merged. Please let us know if you need any help with this.

hyounes4560 commented 2 years ago

I see I was tagged for review, but this PR is also still marked as a draft. Just to confirm, would you like me to review it now? If so, is there anything particular you'd like to be in or out of scope for the first round of review?

It will mark as ready for review; so feel free to leave your feedback when applicable. Thanks

When it comes to things like reST syntax, Sphinx roles, source formatting, and other technical and style guide details, there's a few ways we can handle that, so let me know which you prefer (and we can always change it later on future PRs depending on what works best for you):

Please take a look at my next comment.

  • Review suggestions on the PR, and you apply them
  • Push a commit to the PR taking care of technical nits (all substantive content feedback will still be made as suggestions)
  • Ignore these minor defects on this PR completely and merge with them present, and make an immediate followup PR to address them

In the past, working with our previous writer, I initially did review suggestions, but we subsequently found directly pushing a commit taking care of the small nitpicky stuff to be a lot more efficient for both of us and did that from then on, so that might be the best option, but its ultimately up to you what you want me to do. Please let me know!

It looks like you did a great job overall following one sentence per line, but there was just one line missing a couple line breaks in item 6. Also, I noticed that (probably due to Google Docs) the standard programming quotes were replaced with typographical "smart" quotes; we normally have a pre-commit hook that autofixes them but I had to disable it temporarily due to an issue. It would make the review comments a lot easier to follow if those little issues were cleaned up first; I can take care of it for you in a commit, or you can do it if you prefer—just let me know.

Fixed both issues and the Pre-commit hook works fine now on my machine. Thanks Thanks!

hyounes4560 commented 2 years ago

it might be a better idea to do it incrementally section by section, which each section corresponding to one PR (particularly given you've already aptly titled this one "Update the Editor introduction section", which I presumed was going to be the scope of the PR). This keeps the PRs more manageable, easier for us to review and easier for you to read the feedback, your changes get merged and go live quicker, and you can apply the feedback from one more easily to the next.

This works perfectly for me and that's the plan moving forward. Thanks

hyounes4560 commented 2 years ago

I'll have a followup review focusing on the textual content to keep things organized, once you let me know how you want me to do that. Thanks!

I am not sure what you mean here! do what exactly?

CAM-Gerlach commented 2 years ago

Hey, apologies for the delay and I appreciate your patience; I somehow missed in my notifications that this PR had been updated, and only noticed when I went to check on it manually. I replied to your questions above and the text pass is in work right now.

I am not sure what you mean here! do what exactly?

Sorry for the confusion; that was just the question about whether you preferred those smaller technical items as suggestions, a commit or a separate PR, to which you already let me know over Slack that you'd prefer suggestions for now, which I'll now go ahead with—I didn't want to have too many at once with the image suggestions also here.

Also, one other option for the future I didn't think of—I can also make a PR to your branch which you can then review and potentially incorporate as desired. That's just one other option to have of course; I'll do the suggestions here and going forward unless and until you let me know otherwise. Thanks!

hyounes4560 commented 2 years ago

FYI @ccordoba12 Thanks

CAM-Gerlach commented 2 years ago

I brought this up at the meeting today, and @ccordoba12 said he will review it today or tomorrow, FYI

CAM-Gerlach commented 2 years ago

Thanks to @hyounes4560 for the PR, @steff456 for helping guide her and @ccordoba12 and @jitseniesen for the reviews! This is a big improvement, and looking forward to the next one!