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

Docs: Clarified PR guidelines in maintainer guide #8876

Merged
merged 1 commit into from Jul 7, 2017

Conversation

platinumazure
Copy link
Member

What is the purpose of this pull request? (put an "X" next to item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Modifying PR guidelines in maintainer guide

What changes did you make? (Give an overview)

In the PR guide for maintainers:

  • Separated human verification steps (check that commit tag is correct for issue) from bot verification steps (check that commit tag is present)
  • Removed note about doc-only PRs not requiring an issue, since we only require issues for core changes
  • Added human verification step to check that commit tag is correct for issue/problem
  • Added human verification step to check for presence of issue, if PR is making a change to core
  • Added note that team members can merge chores

Is there anything you'd like reviewers to focus on?

I tried to make changes that were either obvious clarifications/reorganizations, or that reflected our actual practices. Nonetheless, this might need TSC review to make sure I'm not inadvertently proposing to change maintainer guidelines.

@mention-bot
Copy link

@platinumazure, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nzakas and @btmills to be potential reviewers.

@eslintbot
Copy link

LGTM

@platinumazure platinumazure added documentation Relates to ESLint's documentation evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels Jul 4, 2017
@platinumazure
Copy link
Member Author

TSC Summary: Goal is to clarify and improve the PR guidelines in the maintainer guide. I tried to make changes that were either obvious clarifications/reorganizations, or that reflected our actual practices. Nonetheless, this might need TSC review to make sure I'm not inadvertently proposing to change maintainer guidelines.

TSC Question: Are the proposed changes acceptable and accurate?

1. Is the commit summary too long?

The bot will add a comment specifying the problems that it finds. You do not need to look at the pull request any further until those problems have been addressed (there's no need to comment on the pull request to ask the submitter to do what the bot asked - that's why we have the bot!).

Once the bot checks have been satisfied, you check the following:

1. Double-check that the commit message tag ("Fix:", "New:", etc.) is correct based on the issue (or, if no issue is referenced, based on the stated problem).
Copy link
Member

@kaicataldo kaicataldo Jul 5, 2017

Choose a reason for hiding this comment

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

Can we add something like Note that documentation-only pull requests do not require an issue. to the end of this?

@platinumazure
Copy link
Member Author

platinumazure commented Jul 5, 2017 via email

@kaicataldo
Copy link
Member

Sorry - you're right. This LGTM!

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 6, 2017
@btmills
Copy link
Member

btmills commented Jul 6, 2017

These changes were accepted by the TSC in the 2017-07-06 meeting.

@not-an-aardvark not-an-aardvark merged commit 7c8de92 into master Jul 7, 2017
@not-an-aardvark not-an-aardvark deleted the maintainer-guide-review-prs branch July 7, 2017 01:00
@not-an-aardvark not-an-aardvark removed the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Jul 7, 2017
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants