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

stubtest: should unused allowlist entries be an error in stdlib? #6406

Closed
Akuli opened this issue Nov 27, 2021 · 1 comment · Fixed by #6424
Closed

stubtest: should unused allowlist entries be an error in stdlib? #6406

Akuli opened this issue Nov 27, 2021 · 1 comment · Fixed by #6424
Labels
project: policy Organization of the typeshed project

Comments

@Akuli
Copy link
Collaborator

Akuli commented Nov 27, 2021

In third-party stubs, unused allowlist entries cause CI to fail, and must be removed in the same pull request that fixes whatever causes it to be unused.

  • Advantage: super simple
  • Advantage: you can be sure you fixed an allowlist entry when you make a pull request
  • Disadvantage: can be confusing for new contributors
  • Disadvantage: can cause merge conflicts if one pull request removes allowlist entry from line 17 and another removes one from line 18 (I don't think this has ever happened)

In stdlib, unused allowlist entries don't cause CI errors, and are removed in a weeky automatically generated pull request.

  • Advantage: new contributors don't need to worry about the allowlists (more important for stdlib, because there's many allowlists)
  • Advantage: no merge conflicts
  • Disadvantage: complicated
  • Disadvantage: automatic removal doesn't work in all corner cases
  • Disadvantage: automatic removal can fail without anyone noticing it (unlike a CI failure of a pull request)

I like how this works for third-party stubs. Should we do the same for stdlib too?

@Akuli Akuli added the project: policy Organization of the typeshed project label Nov 27, 2021
@hauntsaninja
Copy link
Collaborator

Advantage: you can be sure you fixed an allowlist entry when you make a pull request

Yeah, I've actually found "this didn't get fully fixed but we didn't realise" to be somewhat common, e.g. from today I found that #5388 missed a default arg that I'm fixing in #6417. I've definitely made several mistakes in this vein.

I think my personal standpoint is that as we get to the point where most things in the allowlists are "can't / won't fix", on net erroring for unused allowlist becomes more valuable and less disruptive. And we've already made so much progress towards this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: policy Organization of the typeshed project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants