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.

Sign up for GitHub

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

Info on the peer review process + pull request template #188

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Info on the peer review process + pull request template #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions .github/new_pipeline/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
---
name: New pipeline
description: Initiate the peer review process for a new pipeline
title: "[NEW PIPELINE]: "
labels: ["pipeline", "user contributed", "peer review needed"]
---

> [!IMPORTANT]
> To facilitate the peer review of the new pipeline, do not change the structure of this
> document. Only the parts in comments should be replaced.

## General information about the pipeline

**Title:** <!-- TITLE GOES HERE -->

| Author | Affiliation | GitHub ID | ORCID | [CRediT Role][role] |
|-------|-----|----|----|---|
| | | | | |

[role]: https://credit.niso.org/

> [!INFO]
> Abstract of the pipeline goes here

## Code information

**GitHub repo:** <!-- REPO URL -->

**LICENSE:** <!-- LICENSE NAME HERE -->
Copy link
Contributor

Choose a reason for hiding this comment

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

To remove: This is per script/pipeline, and there could be different licenses in the same PR depending on IP constraints.


> [!NOTE]
> The pipeline license **must** be FOSS, **should** not require the same license for derived
Copy link
Contributor

Choose a reason for hiding this comment

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

ici je mettrais seulement "Open Source", plutôt que FOSS, car c'est la terminologie utilisée dans le MoU qu'on signe ce soir.

> products, and **must** be in either `LICENSE` or `LICENSE.md` in the folder of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, the license is an attribute of the yml file. We expect that people will refer to well-known licences, with version when appropriate. This is a change of paradigm, that allows custom licenses as well.

Do we want that?

> package. You can delete this note when done.

**Languages used:**

- [ ] R (version)
- [ ] Julia (version)
- [ ] Python (version)
- [ ] other

> [!INFO]
> For each *other* language, copy this quote block and list the language, version, and other
> relevant information

**Dependencies manager:** <!-- LIST HERE -->

> [!INFO]
> Pipelines **must** ship with a list of their dependencies, including information about
> which versions are usable. You can delete this note when done.

## Additional information

**Testing:** <!-- free-form text to explain the testing/CI of the pipeline -->

## Optional (recommended)

- [ ] [Mermaid][mermaid] diagram
Copy link
Contributor

Choose a reason for hiding this comment

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

... or other figure


[mermaid]: https://github.blog/developer-skills/github/include-diagrams-markdown-files-mermaid/

## Review information

| Date | Step | Comments | User |
|----|----|----|----|
| <!-- TODAY YYYY-MM-DD --> | Submission | | <!-- YOUR GITHUB ID --> |
| | Initial check | | |
| | Review started | | |
| | Reviewer 1 invited | | |
| | Reviewer 2 invited | | |
Copy link
Contributor

Choose a reason for hiding this comment

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

Most review information and comments should be captured by the history of the PR. Maybe replace this table with a "progress of PR checklist" ?

Progress of Pull Request (to be filled by reviewers):

  • Initial check by
  • Open review call started
  • Reviewers invited,
  • Review 1 completed by
  • Review 2 completed by
  • Modifications received
  • Modifications validated
  • Merged
  • Published on the web

25 changes: 25 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# Guidelines for contribution of new pipelines

> [!TIP]
> The process of submitting a pipeline for inclusion is done entirely through the review of
> a pull request. Most of the pre-review checklist is codified in the "New pipeline" issue
> template. Submissions of new pipelines that do not use this template will not be merged.

## Pre-review checklist

### General information about the pipeline

- [ ] Descriptive title for the pipeline
- [ ] Authorship information with appropriate CRediT roles
Copy link
Contributor

Choose a reason for hiding this comment

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

We would need to add this to the yml description spec in the same pull request. This is surely more precise than the author/reviewer roles that I was proposing, but it would need to be turned upside down since one can have many types : to the author we add the type, and not to the author/reviewer group we add the person.

- [ ] Abstract
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the markdown description that goes along iwth the pipelines, and that we display on the web page? example: https://boninabox.geobon.org/indicator?i=RLI

If yes, we would probably need another checklist for what the abstract should contain. I am weary that this .md abstract duplicates a lot of the information from the yml description file, and that it goes out of sync. The ideal would be that this page is generated by the yml file's info.


### Code checklist
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking such a checklist could go in the top of the PR template (maybe link to it to avoid duplication?):

Before submitting this pull request, I have:

  • Formatted my code: with appropriate indentation, clear variable names, clear function names, comments where needed, etc.
  • Tested my code with the examples
  • Tested my code in various contexts (region, species, time, etc.)
  • Clearly documented the script yml files and in the pipeline metadata tab.
  • Checked that the GitHub actions pass
  • ...


- [ ] Languages/version
- [ ] License
- [ ] Information on testing
- [ ] Information on dependencies

## The review process

## Post-acceptance checklist