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

Update CODEOWNERS #97866

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Update CODEOWNERS #97866

merged 1 commit into from
Oct 10, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Oct 6, 2024

While the codeowner file is in a MUCH better spot than it was before, there was still quite a few edgecases that weren't fully accounted for. This PR attempts to address that by adding a few cases that were missing or weren't using wildcards appropriately. While this PR doesn't account for all of them, it ensures that there's a more appropriate baseline for many of these cases where buildsystem wasn't appropriate

@Repiteo Repiteo added this to the 4.4 milestone Oct 6, 2024
@Repiteo Repiteo requested a review from AThousandShips October 6, 2024 02:35
@Repiteo Repiteo requested a review from a team as a code owner October 6, 2024 02:35
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

I'm not sure if the super groups like @godotengine/_editor actually work as such

@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 9, 2024

I think super groups still work, as _platforms was used before & child members of that can still approve PRs1. I wholly believe they should be used sparingly though, which is why I only added them as baseline/fallback cases.

Footnotes

  1. https://github.com/godotengine/godot/pull/96399#pullrequestreview-2315735412

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

The ones here look good I think, can't speak to missing cases but I think this is good to go

@Repiteo Repiteo merged commit 6a76f18 into godotengine:master Oct 10, 2024
19 checks passed
@Repiteo
Copy link
Contributor Author

Repiteo commented Oct 10, 2024

I HAVE THE POWER

@Repiteo Repiteo deleted the codeowners branch October 10, 2024 19:08
/servers/**/navigation* @godotengine/navigation
/servers/**/physics* @godotengine/physics
/servers/**/rendering* @godotengine/rendering
/servers/**/text* @godotengine/gui-nodes
Copy link
Member

Choose a reason for hiding this comment

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

These filters seem to be too generic: e.g, change to servers/rendering/renderer_rd/storage_rd/texture_storage.cpp trigger /servers/**/text* not /servers/**/rendering* and result in a wrong team assignment.

/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp
/servers/**                              /text*

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants