Skip to content

Add CODEOWNERS for GAP-13#19

Merged
magicmark merged 2 commits intographql:mainfrom
magicmark:add-codeowners
May 4, 2026
Merged

Add CODEOWNERS for GAP-13#19
magicmark merged 2 commits intographql:mainfrom
magicmark:add-codeowners

Conversation

@magicmark
Copy link
Copy Markdown
Contributor

@magicmark magicmark commented May 1, 2026

Add a CODEOWNERS file granting @benjie ownership of the GAP-13 directory.

questions/confirmations for @benjie:

  1. adding even though the directory doesn't exist yet (b/c the PR hasn't been merged; this step is (currently) first
  2. do we want to namespace GAP-N directories in a top level gaps folder? I would think yes actually, but CONTRIBUTING doesn't clarify and package.json encodes that they'd all top level. I think we should change this?
  3. would editors require another person to give approval on this type of PR? (General etiquette in the absence of explicit rules is to not self-merge)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@magicmark magicmark requested a review from benjie May 1, 2026 19:02
@magicmark magicmark marked this pull request as ready for review May 1, 2026 19:03
@benjie
Copy link
Copy Markdown
Member

benjie commented May 2, 2026

Hmmm…

Should GAPS be root level?

Good question.

Pro: simplicity, instant visibility.
Con: messy root.

Gut says you’re right and we should move it to gaps/N/ or gaps/GAP-N/

When to add?

Adding it via GitHub action after merge makes most sense to me. We don’t care that pushed commit won’t have secondary actions fired, it’s admin only.

However it does reveal that the author’s github handle should be in the authors entry - authors should be list of objects maybe?

If done in the current order it should be added to the user’s PR by an editor, if permissions allow. If not, it should be an editor that follows the merge.

Requires approval?

Editors can follow the procedures for admining the repo without needing approval. For merging a gap it might make sense to require “All editor/TSC feedback addressed, plus either two editor/TSC approvals from different organisations, or one approval and a 7-day wait for feedback” but I’m not really sure we need the delay. Probably we should block merging of your own GAP.

@magicmark
Copy link
Copy Markdown
Contributor Author

Editors can follow the procedures for admining the repo without needing approval.

ack, merging this PR.

Gut says you’re right and we should move it to gaps/N/ or gaps/GAP-N/

++, let's go with gaps/GAP-123 (https://github.com/python/peps/tree/main/peps)

@magicmark magicmark merged commit 8d03cf5 into graphql:main May 4, 2026
2 checks passed
@magicmark
Copy link
Copy Markdown
Contributor Author

authors should be list of objects maybe?

makes sense, I like that this implies that CODEOWNERS can always be reproduced by reading all gap metadata.yml files, and any transfer in authorship will be kept up to date in CODEOWNERS too.

and then yeah we can entirely forget about manually maintaining this file and document who adds it and when etc.

magicmark added a commit that referenced this pull request May 5, 2026
Implement changes discussed in #19:

- move GAPs under a `gaps/` parent directory (e.g. `gaps/GAP-123`)
([prior art](https://github.com/python/peps/tree/main/peps))
- Change `authors` in metadata.yml to a list of objects with `name`,
`email`, and `githubUsername`
- CODEOWNERS is now be derived from GAP metadata.
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