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.

Sign up for GitHub

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

Jump to bottom

Update CODEOWNERS #89486

Merged
merged 1 commit into from
Sep 26, 2024
Merged

Update CODEOWNERS #89486

merged 1 commit into from
Sep 26, 2024

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Mar 14, 2024 •
edited by Repiteo
Loading

  • Added shared responsibilities for documentation and tests
  • Made buildsystem manage all build scripts (uniquely)
  • Cleaned up unused cases
  • Added unmanaged cases

TODO:

  • Add shared responsibility for tests
  • Add some missing module cases (needs confirmation on the allocation)
  • Decide on the way to go with buildsystem

Any feedback would be welcome!

My current reasoning is:

  • For tests and documentation shared responsibility is important
  • For buildsystem that's less relevant, we could do it but it'd mean a lot of clutter in this file (the documentation part is cluttered as it is, and haven't added tests yet)

I'm not sure how to verify these changes as they don't show up in the branch (because of organization management etc. they don't turn up on my own branch)

@AThousandShips AThousandShips added this to the 4.3 milestone Mar 14, 2024
AThousandShips
AThousandShips commented Mar 14, 2024
View reviewed changes
.github/CODEOWNERS Outdated
/modules/navigation/ @godotengine/navigation
/modules/gridmap/ @godotengine/3d-nodes
/modules/gridmap/doc_classes/ @godotengine/3d-nodes @godotengine/documentation
/modules/noise/ @godotengine/core
Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't sure what to assign noise to

dalexeev
dalexeev reviewed Mar 14, 2024
View reviewed changes
AThousandShips
AThousandShips commented Mar 14, 2024
View reviewed changes
.github/CODEOWNERS Outdated
/modules/glslang/ @godotengine/rendering
/modules/lightmapper_rd/ @godotengine/rendering
/modules/meshoptimizer/ @godotengine/rendering
/modules/raycast/ @godotengine/rendering
Copy link
Member Author

Choose a reason for hiding this comment

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

Assumed this is where this goes

@AThousandShips
Copy link
Member Author

I can eventually look at cleaning up some more general cases, like fixing scene/2d/physics to belong to 2d-nodes and physics, but I think I'll try keep this simple, the modules part is relatively trivial to rework, but a larger cleanup would be great to help with allocation

@AThousandShips AThousandShips mentioned this pull request Jun 20, 2024
Closed
@akien-mga
Copy link
Member

Added shared responsibilities for documentation and tests

I'm confused by that concept of shared responsibility, I think it might be a misunderstanding of the CODEOWNERS patterns.

The current patterns are not exclusive, but are all taken into account.

So

doc_classes/     @godotengine/documentation 
modules/gltf/    @godotengine/import

Already implies that a change to modules/gltf/doc_classes/*.xml will require a review from both the documentation team and the import team. You can see it in #89444 for example:

image

With only XML changes.

@AThousandShips
Copy link
Member Author

AThousandShips commented Jun 20, 2024 •
edited
Loading

Already implies that a change to modules/gltf/doc_classes/*.xml will require a review from both the documentation team and the import team.

It doesn't seem that way from various other testing, at least the owners for those files is not shared, can you confirm it actually works like that? I haven't been able to in my own testing

The docs part was only because some specific files were just docs

@dalexeev
Copy link
Member

See About code owners - CODEOWNERS syntax:

If you want to match two or more code owners with the same pattern, all the code owners must be on the same line. If the code owners are not on the same line, the pattern matches only the last mentioned code owner.

@AThousandShips
Copy link
Member Author

Take this for example, it modified the docs files in csg but didn't request:

@akien-mga
Copy link
Member

Hm I see, then I misunderstood how it works and we indeed need an approach like this one to be thorough.

@AThousandShips AThousandShips modified the milestones: 4.3, 4.x Jul 7, 2024
Repiteo
Repiteo reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@Repiteo Repiteo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think making buildsystem the de-facto "default" environment makes the most sense. This file would fall under their jurisdiction, so any cases in the future where something isn't accounted for would alert them & subsequently let them update this file as needed.

Also, after reading their documentation, it looks like this file can be moved to the .github folder. Maybe this PR could do that as well to help reduce root bloat, but it isn't strictly necessary.

@AThousandShips

This comment was marked as resolved.

Sign in to view
@Repiteo
Copy link
Contributor

Repiteo commented Jul 26, 2024

Oh, duh, so it is. I must've mistaken CONTRIBUTING.md for it at a glance, my b

@AThousandShips AThousandShips force-pushed the owners_fix branch 2 times, most recently from 80e347e to aa01b74 Compare July 26, 2024 14:59
@akien-mga
Copy link
Member

We've just been discussing this with @Repiteo, I think we should be able to consider this as ready to merge?
It would help a lot to fix the current missing review-requests for areas which the previous CODEOWNERS doesn't handle well.

I wanted to do a quick comparative review with some of the changes I made in #93393 but haven't found the time yet. I don't think there's any really critical difference though so we're probably good to go with this PR as is.

@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 24, 2024 •
edited
Loading

Will do a quick pass over this and fill in some potential misses and otherwise I'll consider this ready, and can go back over it in detail

Will go over it and rebase tomorrow

@AThousandShips AThousandShips marked this pull request as ready for review September 24, 2024 18:14
@AThousandShips AThousandShips requested a review from a team as a code owner September 24, 2024 18:14
@AThousandShips
Copy link
Member Author

AThousandShips commented Sep 25, 2024 •
edited
Loading

Added the new physics modules to the owners but otherwise I'll leave this for future improvements and cleanups, already having a few cases to add and go over

Edit: Decided to add a few more cases to this that follow the same pattern, and will leave some specific cases to discuss later

AThousandShips
AThousandShips commented Sep 25, 2024
View reviewed changes
AThousandShips
AThousandShips commented Sep 25, 2024
View reviewed changes
.github/CODEOWNERS Outdated
/modules/text_server_adv/gdextension_build/ @godotengine/gui-nodes @godotengine/gdextension
/modules/text_server_fb/ @godotengine/gui-nodes
/modules/text_server_fb/doc_classes/ @godotengine/gui-nodes @godotengine/documentation
/modules/text_server_fb/gdextension_build/ @godotengine/gui-nodes @godotengine/gdextension
Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving these for the future, but leaving the spacing for future expansion

@AThousandShips
Update CODEOWNERS
0fdbd22 
* Added shared responsibilities for documentation and tests
* Made buildsystem manage all build scripts (uniquely)
* Cleaned up unused cases
* Added unmanaged cases
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 26, 2024
@akien-mga akien-mga merged commit 506d6e4 into godotengine:master Sep 26, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the owners_fix branch September 26, 2024 16:55
@AThousandShips
Copy link
Member Author

Thank you!

@AThousandShips AThousandShips mentioned this pull request Sep 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Repiteo Repiteo Repiteo left review comments

@dalexeev dalexeev dalexeev left review comments

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
enhancement topic:buildsystem
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@AThousandShips @akien-mga @dalexeev @Repiteo