Skip to content

Establish a super-review policy #9376

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

Closed
brson opened this issue Sep 21, 2013 · 5 comments
Closed

Establish a super-review policy #9376

brson opened this issue Sep 21, 2013 · 5 comments

Comments

@brson
Copy link
Contributor

brson commented Sep 21, 2013

I've been grumbling a lot lately about pull requests that introduce language changes getting through without much review or consensus. At the moment my strategy to prevent this is to just keep an eye on the queue and write something to the effect of 'please wait for further discussion' on things that I think need more review. We need an actually policy though, so that those with review authority know what is expected.

Let's try to keep it minimal. We already have a fair bit of policy, some of which is only loosely followed.

@pnkfelix
Copy link
Member

cc me

@nikomatsakis
Copy link
Contributor

I am also concerned about:

  1. changes to sensitive areas of the compiler, particularly (but not
    limited to) typeck and borrowck.
  2. any core library changes containing unsafe code.

@pnkfelix
Copy link
Member

A thought: from the description, it sounds like super-reviews could primarily target bugs/issues, not for the pull requests themselves. Super-review might not be necessary for the pull requests themselves for pre-approved issues, at least not by default.

(no need to read further if you agree.)

Elaboration on that thought

We may want the policy to differ between pull requests that link to one or more pre-existing, pre-approved issues, versus a pull request that has been put up without a link to the issues that it fixes (or with links solely to issues that have not been acknowledged by the core dev team).

In the former case, I think the reviewer could be given the freedom to judge:

  1. does the patch solves the issue in question,
  2. was there core dev approval on the issue's ticket for a language/API change as part of fixing the issue, and
  3. were there constraints or expected implementation strategies for the solution noted on the linked issues, and if so, does the patch satisfy them?

If the reviewer answers yes for all three questions, then they should not need to wait for super-review.

(In the latter case where a pull request does not link to pre-approved issues, a reviewer might still decide a pull-request can land without super-review; I'm just saying we can put looser standards in place for the former case.)

Such flexibility in policy might help avoid blocking pull requests from landing in the cases where the problem and the path to a solution is known ahead of time.

(And then for modules where the code is known to be tricky, the core dev team can leave notes on the tickets saying that super-review is required for associated pull requests, presumably cc'ing the relevant super-reviewers.)

@steveklabnik
Copy link
Member

Now that we have an RFC process, this can be closed, I believe.

@brson
Copy link
Contributor Author

brson commented Jun 24, 2014

Super could serve a different role from the RFC process, e.g. if you want to merge a change to the typecheck that isn't a change in functionality, super-review would require that someone like Niko sign off on it. Still, this is a vague enough request for process change that I don't think it's buying us anything to have it open.

@brson brson closed this as completed Jun 24, 2014
djkoloski pushed a commit to djkoloski/rust that referenced this issue Sep 21, 2022
…dnet

Add `iter_kv_map` lint

fixes rust-lang#9376

| before | after |
| -------------- | ------------------------- |
| `hmap.iter().map(\|(key, _)\| key)` | `hmap.keys()` |
| `hmap.iter().map(\|(_, v)\| v + 2)` | `hmap.values().map(\|v\| v + 2)` |
| `hmap.into_iter().map(\|(key, _)\| key)` | `hmap.into_keys()` |

Is `MachineApplicable`

changelog: [`iter_kv_map`]: added lint
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

No branches or pull requests

4 participants