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

Proposal: Allow more PRs without issues #7015

Closed
nzakas opened this issue Aug 29, 2016 · 10 comments
Closed

Proposal: Allow more PRs without issues #7015

nzakas opened this issue Aug 29, 2016 · 10 comments
Assignees
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 infrastructure Relates to the tools used in the ESLint development process

Comments

@nzakas
Copy link
Member

nzakas commented Aug 29, 2016

tl;dr I think it's possible that we can now accept more PRs without issues. It will take a bit of work, but I think it would streamline our triaging process.

Background

From the beginning, we've required issues for all pull requests outside of documentation changes. This followed a pattern that I enjoyed at work: where the issue represented the problem and the pull request represented the solution. Some of the benefits I saw in doing this from the onset:

  1. It allowed people to get feedback on something before writing code (I didn't want to reject pull requests, I'd rather reject issues).
  2. We ended up with a record of the discussion because we could tie the commit back to the issue (instead of asking people to open a PR and then update the commit message to include the PR number, which just seemed too cumbersome).
  3. If a pull request was not accepted, a record of the problem (in the form of an issue) was still around so another pull request we wouldn't lose track of it.

Problems

I think this approach served us well at the beginning of the project, but now that we have scaled out a lot, I'm starting to see problems:

  1. People still submit PRs without opening issues. They are nagged into opening an issue. We mark the PR as "do not merge" and it sits there while we discussion it on the issue. This back-and-forth between issue and PR is difficult to keep track of (did you remember to check that the issue is accepted?) and it moves the conversation away from the person who started it. We have a decent number of "do not merge" PRs that get stuck because of this.
  2. We have more issues than we can triage, and some of them are based on PRs that were submitted without an issue. Going back to the first point, this is confusing because there are a bunch of PRs and issues sitting around without any progress and we constantly need to check to see if they are related to one another.
  3. More and more PRs are being left open and unmerged for longer periods of time. This isn't good for bringing on new contributors.

What's Changed?

Anyone who's discussed longstanding practices with me knows that my position is always that we don't revisit past decisions unless something has changed. So, to keep myself honest, here are the things that have changed that I think allow us to make further changes:

  1. GitHub implemented issue and PR templates, so we can actually present people with a form upon submitting the PR to indicate what information we require.
  2. GitHub implemented squash-merges of PRs that automatically append the PR number to the commit message. This gives us the traceability that I was getting after in requiring an issue for each PR.
  3. We are struggling to triage the number of issues that are coming in.

Proposal

Given where we are, I think we can start accepting PRs without issues in some cases. Here's what I have in mind:

  1. Bug Fixes: So long as the PR provides the appropriate information for a bug report, I don't see any reason why we can't accept PRs directly for bug fixes.
  2. New Rules: I'd like to see us require PRs for new rule proposals rather than having people request rules through issues. They'd still need to document their proposal in the PR, but any issues that are opened for rule requests we can just close and ask them to open a PR.
  3. Rule Options: If there's a good explanation in the PR, it seems like we should be able to follow our normal process for accepting rule options through PRs.
  4. Build Related/Chores/Upgrades: Again, so long as there is a good explanation about the PR in the description, I don't see a good reason why we can't accept these directly as PRs.

Things that I think should still require an issue:

  1. Any breaking change (but only if we know it's a breaking change ahead of time - if a PR comes in and we discover it's a breaking change, I don't think we need to open an issue for that, just mark the PR as breaking).
  2. Any change to the core? (Maybe, I'm on the fence about this -- open to feedback)

Obviously, we'd have to update our documentation and PR/issue templates to reflect these changes. That also might require contributors to copy-paste a form into the PR after they've submitted it, but I think that's easily enforceable and a small price to pay to avoid the overhead of opening a new issue.

Pros

  • Should reduce the number of issues that are filed as a result of a PR being submitted first.
  • Definitely reduces the number of "nonsense" issues we file just to get a PR through: upgrades, chores, build-related, etc.
  • People understand that they must implement new rules themselves in order for the rule to be considered for the core.
  • Even if we decide to reject a new rule PR, that person now has a fully-functional rule that they can bundle up and use.
  • Conversations about PRs can happen directly on the PRs instead of bouncing back and forth between issues and PRs.

Cons

  • We may see more PRs that we don't want to merge.
  • We could miss out on good new rule ideas because people can't or won't implement them.
  • We may end up spending more time reviewing PRs.
  • It may be hard to make it clear when it's okay to submit just a PR vs. when you need an issue.

Thoughts?


Edited: Removed the part about requiring PRs for rules as that seems to be the most controversial part of this proposal and isn't required to move forward.

@nzakas nzakas added infrastructure Relates to the tools used in the ESLint development process needs bikeshedding Minor details about this change need to be discussed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 29, 2016
@kaicataldo
Copy link
Member

kaicataldo commented Aug 29, 2016

I'm all for not requiring issues for Build Related/Chores/Upgrades tasks - they always feel redundant.

One thing having an issue for PRs provides is a separation between discussions around what the change should be and the actual implementation. Do you envision the discussion for said issue would all happen in the PR? For example, when a PR comes in for a rule enhancement, would we discuss on the PR and label Do Not Merge until we have discussed and come to a consensus? I guess, in the end, that's not too different from our current way of doing things...

We could miss out on good new rule ideas because people can't or won't implement them.

From the cons list - this was my first thought when reading this, and I think is a legitimate concern.

@kaicataldo
Copy link
Member

kaicataldo commented Aug 29, 2016

Just read through this again and I think I missed the point a bit initially. In that light, I have followup thoughts:

  • How do we know which PRs we should look at that need discussion? Use the same labels we use for issues?
  • Related to my first point, I'm a little concerned about having to manage Issues and also manage some PRs as Issues
  • Not sure I like the idea of requiring a PR for a new rule, but I'm curious to see what others think

@platinumazure
Copy link
Member

platinumazure commented Aug 29, 2016

I'm not sure I like the idea of requiring a PR for a new rule either. I agree it reflects the reality of ESLint (we can't just implement new rules for everyone), but it goes back to the con of "I spent all this time on a PR and it got rejected, what gives?". I think there should still be a relatively risk-free way for people to ask, is this rule something ESLint would even consider. Maybe that way is simply to ask users to visit the chat room, though, I don't know.

👍 to everything else.

@nzakas
Copy link
Member Author

nzakas commented Aug 29, 2016

From the cons list - this was my first thought when reading this, and I think is a legitimate concern.

Yup, definitely. However, our history seems to indicate that it's rare for a user-suggested rule to be implemented by someone other than the person requesting it. Id actually be fine if we didn't add any new rules for a while, so this isn't a big deal to me. We are trying to push people towards custom rules, and I see this as a step in that direction.

How do we know which PRs we should look at that need discussion? Use the same labels we use for issues?

Yup

Related to my first point, I'm a little concerned about having to manage Issues and also manage some PRs as Issues

Can you explain the concern? In theory, people are already looking at both on a regular basis.

@platinumazure risk-free way to ask is the chatroom. Most rule requests are "will you build this for me?" rather than "will you accept a PR for this?".

Everyone - let's not get stuck on just one type of issue right now. This proposal is large, so please give your thoughts on all parts.

@nzakas
Copy link
Member Author

nzakas commented Aug 29, 2016

One thing having an issue for PRs provides is a separation between discussions around what the change should be and the actual implementation. Do you envision the discussion for said issue would all happen in the PR? For example, when a PR comes in for a rule enhancement, would we discuss on the PR and label Do Not Merge until we have discussed and come to a consensus? I guess, in the end, that's not too different from our current way of doing things...

I'd like to stop using "do not merge" and just use "evaluating" like we do for issues. Same issues process, just on PRs.

@kaicataldo
Copy link
Member

kaicataldo commented Aug 29, 2016

Can you explain the concern? In theory, people are already looking at both on a regular basis.

I tend to go through issues and PRs separately when I'm making the rounds, and I think I was having a hard time breaking out of that mindset and worried about having to figure out which PRs "are Issues" and which PRs "are just PRs". The more I think about it, the more that seems like an arbitrary distinction I was making (due to the workflow I've developed), rather than a real problem. Retracting my concern on that point - I'll adapt! :)

@pmcelhaney
Copy link
Contributor

I think this is a great way to reduce the number of open loops, and I think you can simplify the rules around when to open an issue vs. PR.

  1. If you want any kind of change in ESLint (chore, bug fix, feature, whatever), and you're interested in implementing it, go ahead and open a PR. Explain the rationale in the PR just as you would if you were opening an issue. The PR doesn't have to be 100% complete at the time you open it. It could just be a first draft of updating the docs, a new (failing) test, anything that gets the ball rolling and shows you've put some thought into how you're going to approach the problem.
  2. If you want to propose a change in ESLint, but you don't intent to implement it yourself (e.g. you don't know how or you'd rather leave it to a community member), open an issue. Once someone has volunteered and opened a PR, close the issue and let the discussion move to the PR. The PR should start with a link to the issue and a summary of whatever discussion happened there.
  3. If you're toying with an idea and you want to bounce it off people, go to Gitter first.
  4. If you have a question, ask in Gitter.

Contributors should understand that a PR is considered guilty until proven innocent. The team is not obliged to accept it. If the rationale and general approach is not explained in the PR, no one should be expected to review code changes.

Changes to the core and breaking changes get more scrutiny, but I don't see any value in having that scrutiny take place in an issue rather than a PR -- especially considering that we often don't know a change is breaking until some discussion has taken place.

@nzakas
Copy link
Member Author

nzakas commented Aug 30, 2016

@pmcelhaney well said, and all really good points. I'm not sure I agree that an issue should be closed as soon as a PR related to it is opened, as I think there's value in having the PR merge trigger the issue being closed.

For everyone, I've removed the part about requiring a PR for new rules since that's the thing people seem to be getting caught up on and it's not overly important to the proposal. Let's just say new rules can come in as a PR or issue.

Okay, I don't like doing this very often but this is a big change that requires the whole team to be on board, so @eslint/eslint-team please review and comment if you haven't already done so.

@gyandeeps
Copy link
Member

Every thing sounds good to me.

Any change to the core?

I think we should require issue of any core changes so that we can have discussion before making changes.

@nzakas
Copy link
Member Author

nzakas commented Aug 31, 2016

Okay, lots of thumbs up on this, so I'm going to assume there are no significant objections, thus marking as accepted. I'll update the documentation when I have energy and pass that around for final approval before we officially implement this.

@nzakas nzakas self-assigned this Aug 31, 2016
@nzakas nzakas 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 Aug 31, 2016
@alberto alberto added documentation Relates to ESLint's documentation and removed needs bikeshedding Minor details about this change need to be discussed labels Sep 3, 2016
@nzakas nzakas closed this as completed in 457be1b Sep 9, 2016
@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 infrastructure Relates to the tools used in the ESLint development process
Projects
None yet
Development

No branches or pull requests

6 participants