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

Add CODEOWNERS file #7216

Closed
wants to merge 4 commits into from
Closed

Conversation

triallax
Copy link
Contributor

@triallax triallax commented Oct 9, 2021

What is it?

  • Meta improvement to the project (dev facing)

Description of the changes in your PR

See https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners for how this works. The hope is that this will lead to more efficient PR triaging and less delays for PRs to be merged.

For now, I assigned myself .github and the READMEs, and @Redirion the player code (I hope you're fine with that :) ). I'll be waiting for the other team members to state what parts of the repo they want to "own," so this will be a draft for now.

I have no idea why those extra commits are there; please squash them when merging.

Due diligence

@triallax triallax added the meta Related to the project but not strictly to code label Oct 9, 2021
@triallax triallax marked this pull request as ready for review October 9, 2021 19:25
@triallax triallax marked this pull request as draft October 9, 2021 19:25
@litetex
Copy link
Member

litetex commented Oct 10, 2021

Okay so this is fairly new to me.
So a bit of personal thoughts here:
At first, I don't know if it makes sense to "own" parts of the code.

What happens if the person is on vacation or otherwise not reachable (inactive)?
What happens if we e.g. refactor packages and now they are on a completely other place? Do we have to rewrite the whole file then?
Who keeps the file up-to-date?
We also have sometimes >30 classes in a package - all written by different devs. Should we label each class/file here? (That would be a lot of work)
Who is the code owner of e.g. the Player-class? So many people have made changes there...

In my opinion the problem is that we don't have enough time/people to review PRs.
Usually the ~20 open inactive PR which I clean up a week ago got closed because,

So most issues got closed because they were either controversial and got stuck in that state or the author failed to reply.

Maybe it would be better if someone would review old PRs and either kill or merge/request changes on them each month (like I did).

I also found this after a bit of searching the web:
https://softwareengineering.stackexchange.com/questions/108019/code-owner-system-is-it-an-efficient-way
Most upvoted answer: https://softwareengineering.stackexchange.com/a/108021

I have nothing against trying to introduce it and see if it works, but if it doesn't we should remove it.

@Redirion
Copy link
Member

I think the general idea / purpose of this is to take some burden from the project lead (TobiGr). As of now everyone seeks Tobis advice / decision on anything that affects any part of NewPipe. By introducing some divide and conquer we don't introduce hard borders or create roles that overrule anyone else. It is more like that owners are a "person to contact" for this specific section and that this person is most likely active and familiar with issues and PRs regarding this section so that people can ask those instead of TobiGr. And of course owners should feel responsible of reviewing PRs that are within their section.

I also find the player package rather huge. It might be useful to have more than one owner for such big sections if we can't find useful further divide possibilities.

@triallax
Copy link
Contributor Author

triallax commented Oct 10, 2021

@litetex I think there is some misunderstanding here. I put "own" in quotes for a reason: a code owner doesn't really own the code; the point is just that their review will be requested on a PR that modifies files that they "own," and their review isn't even required to merge the PR. Also, like @Redirion already stated, it would help reduce the burden on Tobias by making it clearer for people whom to get help from if they ever get stuck. The point here isn't to revolutionize the process.

I also find the player package rather huge. It might be useful to have more than one owner for such big sections if we can't find useful further divide possibilities.

Yeah, you're right; I'm open to any suggestions.

@opusforlife2
Copy link
Collaborator

Nice! I didn't even think of this solution, even though I've seen it happening over at the Fenix repo.

Regarding doubts about its effectiveness, we can always revert the change if it doesn't seem to be working for us. But there is no harm in trying it out, is there?

@triallax
Copy link
Contributor Author

triallax commented Feb 5, 2022

Seeing as this PR didn't really progress at all since it was opened, I'm closing it. if anybody's interested in this though, feel free to make a comment.

@triallax triallax closed this Feb 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to the project but not strictly to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants