Add pull request template #50
Conversation
There was a problem hiding this comment.
Hi @dccowan! This looks great! I just left a few comments below that would improve the rendering of the Markdown, fix some undefined references, and some minor corrections to the text.
I think the biggest comment I have is regarding the requirements for contributions. Find them below in one the comments, feel free to reply there.
| * [ ] Introduction is complete: | ||
| * [ ] Title and author added to notebook | ||
| * [ ] Admonitions for notebook difficulty and computational resources have been added | ||
| * [ ] Keywords list has been added | ||
| * [ ] Summary paragraph describing the tutorial has been added | ||
| * [ ] Learning objectives have been listed | ||
| * [ ] Hyperlinks to other tutorial notebooks added if necessary |
There was a problem hiding this comment.
What about simplifying the text in these items? Something like this:
| * [ ] Introduction is complete: | |
| * [ ] Title and author added to notebook | |
| * [ ] Admonitions for notebook difficulty and computational resources have been added | |
| * [ ] Keywords list has been added | |
| * [ ] Summary paragraph describing the tutorial has been added | |
| * [ ] Learning objectives have been listed | |
| * [ ] Hyperlinks to other tutorial notebooks added if necessary | |
| * [ ] Introduction is complete and includes: | |
| * [ ] Title and author | |
| * [ ] Admonitions for notebook difficulty and computational resources | |
| * [ ] Keywords list | |
| * [ ] Summary paragraph describing the tutorial | |
| * [ ] Learning objectives | |
| * [ ] Hyperlinks to other tutorial notebooks (if necessary) |
.github/pull_request_template.md
Outdated
| * [ ] Links to API documentation added for all classes and functions that are used | ||
| * [ ] Newly introduced functionality is explained or links provided to relevant notebooks | ||
| * [ ] All figures are legible and rendered appropriately | ||
| * [ ] Coding cells have been linted according to the [style guides](https://docs.simpeg.xyz/latest/content/getting_started/contributing/code-style.html). |
There was a problem hiding this comment.
We have style guide for this particular repo. I'd suggest linking at those instead of linking to the ones we have in SimPEG.
| * [ ] Coding cells have been linted according to the [style guides](https://docs.simpeg.xyz/latest/content/getting_started/contributing/code-style.html). | |
| * [ ] Coding cells have been linted according to the [style guides](https://simpeg.xyz/user-tutorials/formatting#check-style-of-notebooks). |
| These are provided on the [Tutorial Structure and Formatting Requirements](contributing/formatting.md) page. | ||
|
|
||
| **Step 3: The Review Process** | ||
|
|
There was a problem hiding this comment.
Add this missing reference so the ones using it in this file render properly:
| [mystmd.org]: https://mystmd.org |
| Steps for Adding a Tutorial | ||
| --------------------------- | ||
|
|
||
| As explained on the [SimPEG user tutorials](../index.md) landing page, the SimPEG user tutorials are a library of [Jupyter Notebooks](https://jupyter.org/) that have been published as a website using [MyST](https://mystmd.org/). To add a tutorial notebook to the project, you will need to complete the following steps: |
There was a problem hiding this comment.
And now we defined it, we can use it here:
| As explained on the [SimPEG user tutorials](../index.md) landing page, the SimPEG user tutorials are a library of [Jupyter Notebooks](https://jupyter.org/) that have been published as a website using [MyST](https://mystmd.org/). To add a tutorial notebook to the project, you will need to complete the following steps: | |
| As explained on the [SimPEG user tutorials](../index.md) landing page, the SimPEG user tutorials are a library of [Jupyter Notebooks](https://jupyter.org/) that have been published as a website using [MyST][mystmd.org]. To add a tutorial notebook to the project, you will need to complete the following steps: |
| the website: | ||
|
|
||
| ```bash | ||
| msyt start |
There was a problem hiding this comment.
Fix typo:
| msyt start | |
| myst start |
| In this cell, the contributor must provide a set of relevant keywords. E.g. | ||
|
|
||
| ``` | ||
| **Keywords:** gravity inversion, sparse-norm inversion, integral formulation, tree mesh. |
There was a problem hiding this comment.
Just sharing an idea, not to tackle it now: MystMD has support for specifying keywords through the frontmatter. I think at some point it would be better to use those instead of hard-coding the keywords as Markdown text.
| Please ensure that the contents of your tutorial **DOES NOT** fall into any of the following categories: | ||
|
|
||
| * the tutorial focusses on functionality that is not part of SimPEG | ||
| * the amount of SimPEG functionality NOT already covered in another tutorial is insufficient |
There was a problem hiding this comment.
Is there a better way to make this statement? It has three negations (the DOES NOT from above) plus the "NOT" and "insufficient". I think it's hard to understand the point.
| * the tutorial focusses on functionality that is not part of SimPEG | ||
| * the amount of SimPEG functionality NOT already covered in another tutorial is insufficient | ||
| * the tutorial focusses on a data-specific result and is not generalizable | ||
| * the tutorial was created for the purpose of self promotion |
There was a problem hiding this comment.
I struggle a little bit with this point. If any contributor wants to add a tutorial and the tutorial is great, the reasons why they want to push it should not matter too much in order to decide whether we want to merge it or not. Also, how can we prove that a given contributor opened the PR with the purpose of self promotion? Answers to that question can only be subjective.
I'd be inclined to remove this point, or keep it but provide a clear definition of "self promotion" that could be objectively determined.
What do you think? What type of contributions is this rule trying to avoid?
|
|
||
| Tutorials that fall into any of these categories will not be accepted! |
There was a problem hiding this comment.
I think this is a strong statement that could very likely prevent contributors from opening a PR or wanting to contribute.
What if we add a statement like this one instead:
In case you have an idea, but are unsure if it satisfies or not these criteria, feel free to open an Issue describing it in detail we can discuss it.
(Feel free to suggest changes to that one, it's just an example).
To ensure consistency, we should add a PR template for new notebooks. I have added a PR template. The checklist should ensure that all notebook requirements are met.