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

add a template for pull request #19

Merged
merged 11 commits into from
Apr 3, 2020
Merged

add a template for pull request #19

merged 11 commits into from
Apr 3, 2020

Conversation

SarahAlidoost
Copy link
Contributor

@SarahAlidoost SarahAlidoost commented Mar 17, 2020

It adds a PR template containing a list of tasks. It closes #18

@SarahAlidoost SarahAlidoost marked this pull request as ready for review March 17, 2020 13:53
@ledm ledm mentioned this pull request Mar 17, 2020
12 tasks
@bouweandela bouweandela requested a review from ledm March 18, 2020 10:47
@JaroCamphuijsen
Copy link
Contributor

When I ran the PR template through a markdownlint tool I got:

these comments

21 - MD012 Multiple consecutive blank lines [Expected: 1; Actual: 2]
9 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
10 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
11 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
12 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
13 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
14 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
18 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
19 - MD030 Spaces after list markers [Expected: 1; Actual: 3]
7 - MD036 Emphasis used instead of a heading [Context: "Tasks"]

@JaroCamphuijsen
Copy link
Contributor

Looks good to me. What do you think @ledm , let's merge?

@ledm
Copy link
Contributor

ledm commented Mar 19, 2020

Looks good to me, only very minor comments. Nice work.

@ledm
Copy link
Contributor

ledm commented Mar 20, 2020

We probably want to add a list of bullet points for PRs containing new lessons too:

  • Lesson is clearly explained, and includes questions and objectives.
  • All code instructions have been tested.
  • Lesson is a reasonable length. If its very long, please consider splitting it into two parts.
  • Lesson ends with a summary of the key points.

Just some ideas, but think it would be useful.

@SarahAlidoost
Copy link
Contributor Author

@JaroCamphuijsen @ledm Thanks for your comments.
There are not any recommendations regarding a linter and spell/grammar checker by Carpentry.

Regarding the linter tool for example, in vscode, a markdownlint can be installed as an extension. Users don’t need to run a command line for that. However, none-vscode users can install another tool via gem install mdl, and run mdl markdown_filename.
Regarding the spell/grammar checker, an option is Grammarly vscode extension. It depends on the editors that is used to develop lesson material. I will ask other colleagues for more suggestions.
The contributing.md is the best place to explain about both linter and spell/grammar checker. I update this PR template accordingly.

@SarahAlidoost
Copy link
Contributor Author

SarahAlidoost commented Mar 20, 2020

Thanks a lot. Indeed very useful items. Please see below:

We probably want to add a list of bullet points for PRs containing new lessons too:

  • Lesson is clearly explained, and includes questions and objectives.
    it will be covered in the lesson formatting section in contributing.md
  • All code instructions have been tested.
    I added this item to the PR template.
  • Lesson is a reasonable length. If its very long, please consider splitting it into two parts.
    it will be covered in the lesson formatting section in contributing.md
  • Lesson ends with a summary of the key points.
    it will be covered in the lesson formatting section in contributing.md

Just some ideas, but think it would be useful.

For more info, please see https://carpentries.github.io/lesson-example/04-formatting/index.html

- [ ] If you are contributing to the lesson materials, make sure the content does not have spelling or grammar errors. See `Lesson development` in [CONTRIBUTING.md](https://github.com/ESMValGroup/tutorial/blob/master/CONTRIBUTING.md).
- [ ] Please use a `markdown lint tool` to check that your files do not contain errors. See `Lesson development` in[CONTRIBUTING.md](https://github.com/ESMValGroup/tutorial/blob/master/CONTRIBUTING.md).
- [ ] Preview changes on your own machine before pushing them to GitHub by running `make serve`, alternatively `make docker-serve`. See `Lesson development` in[CONTRIBUTING.md](https://github.com/ESMValGroup/tutorial/blob/master/CONTRIBUTING.md).
- [ ] All code instructions have been tested.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to make sure that these have been tested by the author and the reviewer?

Suggested change
- [ ] All code instructions have been tested.
- [ ] All code instructions have been tested by the PR author.
- [ ] All code instructions have been tested by the reviewer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ledm Thanks. I think that by default both reviewers and authors should check all the items provided in this template. So, we don't need to mention reviewer or author for this specific item.

SarahAlidoost and others added 3 commits March 23, 2020 18:51
Co-Authored-By: Lee de Mora <ledm@pml.ac.uk>
Co-Authored-By: Lee de Mora <ledm@pml.ac.uk>
Co-Authored-By: Lee de Mora <ledm@pml.ac.uk>
@JaroCamphuijsen
Copy link
Contributor

Apart from the tiny comment I left, I think this is ready to merge!

SarahAlidoost and others added 2 commits March 24, 2020 10:06
Co-Authored-By: Jaro Camphuijsen <jjecamphuijsen@gmail.com>
@SarahAlidoost SarahAlidoost requested a review from ledm March 25, 2020 15:53
@SarahAlidoost
Copy link
Contributor Author

@ledm @JaroCamphuijsen I think the template is ready, Shall we merge this PR?

@bouweandela bouweandela merged commit ba44db7 into master Apr 3, 2020
@bouweandela bouweandela deleted the add_PR_template branch April 3, 2020 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a pull request template
4 participants