Revise Pull Request review process in CONTRIBUTING.md

[weiji14]

Description of proposed changes

To make it easier for new contributors submitting a Pull Request to PyGMT, the changes here will revise our CONTRIBUTING.md documentation to be more clear on all the steps from getting a Pull Request opened to being merged.

I.e. It should answer the question: What should a new contributor know about when submitting a Pull Request to PyGMT?

We should strike a balance between being detailed enough to cover the essential steps, and yet still keep it succinct so as not to overwhelm people with a big checklist of tasks to do. Keep it focused on the point of view of a new Contributor!

Resources (from #1113):

• https://numpy.org/devdocs/dev/reviewer_guidelines.html
• https://tljh.jupyter.org/en/latest/contributing/code-review.html
• https://xarray.pydata.org/en/stable/contributing.html#where-to-start
• https://github.com/joho/awesome-code-review

Addresses one component of #1113

Reminders

• Run make format and make check to make sure the code follows the style guide.
• Add tests for new features or tests that would have caught the bug that you're fixing.
• Add new public functions/methods/classes to doc/api/index.rst.
• Write detailed docstrings for all functions/methods.
• If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

• /format: automatically format and lint the code
• /test-gmt-dev: run full tests on the latest GMT development version

[core-man]

I also have some suggestions about the Documentation sections (I think I cannot make suggestions directly at the original file). I am sorry those suggestions are not directly related to the PR itself.

---

See lines 558-561:

All docstrings should follow the numpy style guide. All functions/classes/methods should have docstrings with a full description of all arguments and return values.

This is simply repeated to the Docstrings subsection, and it also misses the second point about 79 chars.

All docstrings should follow the numpy style guide. All functions/classes/methods should have docstrings with a full description of all arguments and return values.

While the maximum line length for code is automatically set by Black, docstrings must be formatted manually. To play nicely with Jupyter and IPython, keep docstrings limited to 79 characters per line.

So, shall we just add a link to the Docstrings subsection? e.g.,

Please refer to the Docstrings subsection for docstrings rules.

Or we can remove this sentence.

---

GMT reStructuredText Cheatsheet is great. Shall we a link in the Documentation section?

[maxrjones]

I am wondering about whether these two points should go into this PR or a separate PR:

#1190 (comment) (For naming aliases): "Use an underscore to separate words bridged by vowels (aeiou), e.g. no_skip, z_only, etc. Don't use underscore for others, e.g. distcalc, crossprofile (I'll change this) and coltypes."

#1145 (review) (For PRs that wrap modules): Break up the wrapping of module aliases into multiple PRS.

[weiji14]

Thanks team for all the suggestions, great to see lots of ideas on making PRs better in PyGMT!

@core-man, since you have write access to the PyGMT repo now, do you want to go ahead and make direct commits in this PR? I know you're busy with job applications at the moment, and I'm juggling quite a few things as well (PRs, paper writing), but once you have time, feel free to push changes directly to this 'revise-pr-review-process' branch, and I'll try to keep up on top of things too in the next few weeks.

[core-man]

Thanks team for all the suggestions, great to see lots of ideas on making PRs better in PyGMT!

@core-man, since you have write access to the PyGMT repo now, do you want to go ahead and make direct commits in this PR? I know you're busy with job applications at the moment, and I'm juggling quite a few things as well (PRs, paper writing), but once you have time, feel free to push changes directly to this 'revise-pr-review-process' branch, and I'll try to keep up on top of things too in the next few weeks.

Okay. I'll work on it next week.

[core-man]

I've performed the following revisions since I began to work on this PR. Ping @weiji14 for some comments before I further polish them.

Code Review

• d0e8fe7: Use nested lists for the Code Review section.
• 81d6285 and 8d02adb: Move the Code Review section to be within the General Guidelines section
• d1977b0: Add some section links in the Code Review section

General guidelines for making a PR

• a74babe: Improve the Open an issue first rule.
• 3e1124a: Use nested lists for the PR guidelines

Documentation

• 4a4393a: Remove the docstrings part in Cross-referencing with Sphinx
• c700488: Improve Building the documentation

[weiji14]

I've performed the following revisions since I began to work on this PR. Ping @weiji14 for some comments before I further polish them.

Thanks @core-man for the great work! Sorry for coming back to this so late, I've had a scan through your changes and it looks much improved. Only made one tiny typo change at 8b716a6, but otherwise, I think it's at a stage that's ready to review by the wider team 😄

[maxrjones]

Nice work! In my opinion, much of "General Guidelines" section under "Contributing Code" applies to both code- and documentation-focused PRs. What do you think about this section being before "Editing the Documentation" and "Contributing Code"?

[seisman]

Looks good to me.

[weiji14]

Nice work! In my opinion, much of "General Guidelines" section under "Contributing Code" applies to both code- and documentation-focused PRs. What do you think about this section being before "Editing the Documentation" and "Contributing Code"?

You're quite right. I've had a think about moving "General Guidelines" up before "Editing the Documentation", but it seems a bit intimidating if we start explaining what a Pull Request is when someone just wants to submit a typo fix. So I would keep the "Editing the Documentation" section at the top since it's where most people making a simple contribution will start, and leave the "Contributing Code"-"General Guidelines" section below it for more advanced user contributions. I.e. keep the current state.

That said, I'm open to reorganizing the whole CONTRIBUTING.md documentation in a follow-up PR, once this one (focused on the PR review process) is completed. You raise an important point that there is room to streamline the new-contributor workflow a bit more.

[core-man]

That said, I'm open to reorganizing the whole CONTRIBUTING.md documentation in a follow-up PR, once this one (focused on the PR review process) is completed. You raise an important point that there is room to streamline the new-contributor workflow a bit more.

Great. I think the current CONTRIBUTING.md is friendly for experienced contributors, while a new contributor may always be a little confused with such a long list.

[seisman]

@weiji14 Merge this PR?

[weiji14]

@weiji14 Merge this PR?

Check with @core-man, any other changes you want to make?

[core-man]

@weiji14 Merge this PR?

Check with @core-man, any other changes you want to make?

I am okay to merge it. Looks nice.