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

Project: Update code owners (Parser modules, RichText component) #13820

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 11, 2019

This pull request seeks to make a few additions to the code owners file:

The latter of these is interesting to implement. Since at most one code owner match can be made in the CODEOWNERS file, it's necessary to duplicate the entries from the /packages/editor "fallback". I think it may be more something to consider more often to include ownership over more granular paths within a package (e.g. specific editor components, or specific blocks), but hand-crafting these is potentially error-prone. It may be worth considering some automation of CODEOWNERS being directory-specific, and aggregated up through ancestry to the top-level directory (e.g. combine packages/editor/src/components/rich-text/CODEOWNERS, packages/editor/CODEOWNERS to generate the root CODEOWNERS file)

@aduth aduth added the [Type] Project Management Meta-issues related to project management of Gutenberg label Feb 11, 2019
@aduth aduth requested review from ellatrix and dmsnell February 11, 2019 19:16
@nerrad
Copy link
Contributor

nerrad commented Feb 11, 2019

It may be worth considering some automation of CODEOWNERS being directory-specific, and aggregated up through ancestry to the top-level directory (e.g. combine packages/editor/src/components/rich-text/CODEOWNERS, packages/editor/CODEOWNERS to generate the root CODEOWNERS file)

I like how this idea allows for making things really granular. The downside however, is the maintenance overhead is greater for remembering what file people have their "ownership" applied in. This maybe could be mitigated by any automation adding a comment block listing the path the ownership was retrieved from.

@aduth aduth force-pushed the update/code-owners-parser-rich-text branch from eaf401f to 36fe4c9 Compare February 11, 2019 19:46
@aduth
Copy link
Member Author

aduth commented Feb 11, 2019

I discovered the RichText entry had already been added in #13734, under the "Rich Text" categorized heading (rather than as a sibling following the entry for /packages/editor).

On this point, I commented at #13734 (comment) and #13734 (comment) with some concerns about how we currently categorize things:

I think it could speak to the difficulty in maintaining this file when the entries are categorized the way they are (vs. a file listing) since I was inclined to add it after the existing /packages/editor entry.

Another issue we'll encounter with organizing file paths into categories is that order has an impact for priority, and it'll be hard to maintain both prioritization while keeping things organized within categories.

@ellatrix
Copy link
Member

I'm fine with moving the line to wherever you'd like. :)

@gziolo
Copy link
Member

gziolo commented Feb 12, 2019

Since we discuss some changes. I would also love we create a new section where people interested in reviewing *.scss files could opt in. In many cases I got notifications about such changes and I would be fine skipping them as usually, they require some design feedback anyways.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Fine as is with @dmsnell added :)

@aduth
Copy link
Member Author

aduth commented Feb 12, 2019

I would also love we create a new section where people interested in reviewing *.scss files could opt in. In many cases I got notifications about such changes and I would be fine skipping them as usually, they require some design feedback anyways.

Is it the sort of thing where we adopt what we did with native files, assigning them to @ghost until someone chooses to take ownership?

@aduth aduth merged commit 36efc71 into master Feb 12, 2019
@gziolo gziolo deleted the update/code-owners-parser-rich-text branch February 13, 2019 06:05
@gziolo
Copy link
Member

gziolo commented Feb 13, 2019

Yes, I think it would be good for start. I’ll open PR today. 👍

@gziolo
Copy link
Member

gziolo commented Feb 13, 2019

Opened #13854 as discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Project Management Meta-issues related to project management of Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants