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

ENH: Rework the PR template and first time contributor message #622

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

jhlegarreta
Copy link
Contributor

@jhlegarreta jhlegarreta commented Aug 22, 2023

Rework the PR template and first time contributor message:

  • Edit and reword as necessary the PR template and first time
    contributor message config files so that the first time contribution
    information is not duplicated.
  • Instruct new contributors to add their names to the list in
    alphabetical order: reorder contributors alphabetically, keeping
    Cieslak, Matthew as first and and Satterthwaite, Theodore D. as last
    authors.
  • Make the order of the contributor fields consistent: name,
    affiliation, orcid.
  • Slightly reword the first time contributor placeholder tags in the
    first time contributor message config file.
  • Remove the mention to Russell Poldrack as an author of the tool from
    the PR template: Russell is not actually listed as an author in the
    .zenodo.json file. Most likely this was transferred as-is from the
    fmriprep pipeline github config file:
    nipreps/fmriprep@2a25395
  • Slightly reword the first time contributor placeholder tags in the
    first time contributor message config file.

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Aug 22, 2023

A few notes:

  • Although I am modifying the .github/config.yml file as well, the template that is actually being used, as shown in the screenshot below, is docs/pull_request_template.md, so probably we should keep one of the two to reduce maintenance effort and avoid having mismatching information.
  • Is adding new contributors at the end of the list desired over other choices? Maybe alphabetical sorting would be easier to maintain (having an exception on some names, e.g. first/last authors)?
  • I have noticed that Sydney appears twice in the list, with two affiliations: https://github.com/PennLINC/qsiprep/blame/e528cad5bf4d4ed6b528747ea3302120859a22f7/.zenodo.json#L9 and https://github.com/PennLINC/qsiprep/blame/e528cad5bf4d4ed6b528747ea3302120859a22f7/.zenodo.json#L55. Should these be merged into a single entry?
    Following an alphabetical sorting would have probably prevented this.
  • I have noticed that different contributors/commits have followed a different order when using the fields. Which is the preferred order?

I would add additional commits to the current PR to address the above issues when answered.

pr_template

@mattcieslak
Copy link
Collaborator

All great catches here @jhlegarreta! Sydney should only be in once and we should probably keep the middle authors (after Phil and before Ted) alphabetized.

I also like your idea to use a PR template in only one place. If you have a preference, I'm happy to follow

@jhlegarreta
Copy link
Contributor Author

@mattcieslak Thanks. Will modify the commit in the current branch then. Making it a draft for now.

@jhlegarreta jhlegarreta marked this pull request as draft August 23, 2023 17:03
Rework the PR template and first time contributor message:
- Edit and reword as necessary the PR template and first time
  contributor message config files so that the first time contribution
  information is not duplicated.
- Instruct new contributors to add their names to the list in
  alphabetical order: reorder contributors alphabetically, keeping
  Cieslak, Matthew as first and and Satterthwaite, Theodore D. as last
  authors.
- Make the order of the contributor fields consistent: name,
  affiliation, orcid.
- Slightly reword the first time contributor placeholder tags in the
  first time contributor message config file.
- Remove the mention to Russell Poldrack as an author of the tool from
  the PR template: Russell is not actually listed as an author in the
  `.zenodo.json` file. Most likely this was transferred as-is from the
  `fmriprep` pipeline github config file:
  nipreps/fmriprep@2a25395
- Slightly reword the first time contributor placeholder tags in the
  first time contributor message config file.
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Aug 26, 2023

Push forced with changes:

  • Although I am modifying the .github/config.yml file as well, the template that is actually being used, as shown in the screenshot below, is docs/pull_request_template.md, so probably we should keep one of the two to reduce maintenance effort and avoid having mismatching information.

Reworked both files to avoid having duplicate information as much as possible.

  • Is adding new contributors at the end of the list desired over other choices? Maybe alphabetical sorting would be easier to maintain (having an exception on some names, e.g. first/last authors)?

Adopted alphabetical ordering; kept Matt as first; kept Ted as last.

  • Left Sydney's Perelman affiliation following her orcid. Please amend if necessary.
  • Following an alphabetical sorting would have probably prevented this.

Adopted alphabetical ordering.

  • I have noticed that different contributors/commits have followed a different order when using the fields. Which is the preferred order?

Adopted name, affiliation, orcid.

we should probably keep the middle authors (after Phil and before Ted) alphabetized.

Phil is not there. I assume you mean Philip Cook, but no Phil/Philip is on the list.

@jhlegarreta jhlegarreta marked this pull request as ready for review August 26, 2023 18:07
@jhlegarreta jhlegarreta changed the title DOC: Remove mention to Russell Poldrack as author from the PR template ENH: Rework the PR template and first time contributor message Aug 26, 2023
@mattcieslak
Copy link
Collaborator

This looks great, anything else you'd like to add or can I merge? Thanks!!

@jhlegarreta
Copy link
Contributor Author

This looks great, anything else you'd like to add or can I merge? Thanks!!

You can go ahead and merge Matt. In order to get the new contributor message working the https://github.com/apps/welcome app needs to be installed in the organization/repository. Only administrators of the organization/repository can do that.

@mattcieslak mattcieslak merged commit 8942e25 into PennLINC:master Aug 31, 2023
2 checks passed
@jhlegarreta jhlegarreta deleted the RemoveRPFromPRTemplate branch August 31, 2023 18:47
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.

2 participants