Add initial version of a CODEOWNERS mapping

[wilzbach]

GitHub recently introduced code owners.

Code owners are automatically requested for review when someone opens a pull request that modifies code that they own. When someone with admin permissions has enabled required reviews, they can optionally require approval from a code owner.

Moreover, it's a simple plain-text format:

A CODEOWNERS file uses a pattern that follows the same rules used in gitignore files. The pattern is followed by one or more GitHub usernames or team names using the standard @username or @org/team-name format.

For more information, the help page goes into more details.

We were actually looking to implementing sth. similar with the Dlang-Bot, so I am very happy that GitHub helped us out here. I have thrown together a basic, initial CODEOWNERS file. It's incomplete and I can only encourage everyone to step up and adopt a module. There are no strings attached, but newcomers immediately get a helpful point of contact.

@andralex,@burner,@CyberShadow,@DmitryOlshansky,@don-clugston-sociomantic,@jmdavis,@jpf91,@klickverbot,@kyllingstad,@MartinNowak,@schveiguy,@WalterBright,@WebDrake,@ZombineDev - are you all ok with being listed here? Would you like to adopt more modules? :)

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

[CyberShadow]

Praise be.

Please gimme std.file and the std.windows / std.c.windows ones.

I could also take std.stdio (together with @schveiguy), std.json, std.path, etc.c.*, zip/zlib, and the curl ones I guess.

[JackStouffer]

I'll take std.conv and std.string

[JackStouffer]

Walter is too busy with other stuff to review Phobos PRs. Best leave him off.

[CyberShadow]

@wilzbach Could we get a listing of all modules so we see the unassigned ones?

[JackStouffer]

I guess I'll also volunteer for std.array and std.algorithm.iteration

[wilzbach]

Awesome! Thanks a lot already!

@wilzbach Could we get a listing of all modules so we see the unassigned ones?

Yes. I added the unassigned files and commented them.

[andralex]

I should be on hook for all of std/, at least for the time being. Thanks!

[dnadlinger]

Great initiative, thanks!

I'd also be happy to take a share in std/math*, having recently gone through all of it for AArch64 support.

[wilzbach]

@JackStouffer: I though it makes more sense to group entire std.algorithm - still interested?

[JackStouffer]

sure

[wilzbach]

Maybe @rainers and @MartinNowak ?

[rainers]

+#std/demangle.d
Maybe @rainers and @MartinNowak ?

Fine with me. This is just a wrapper for core.demangle, though.

[wilzbach]

I should be on hook for all of std/, at least for the time being. Thanks!

@andralex AFAICT the CODEOWNERS file will ask for a reviewer for the last matching line, hence this line is a fall-through (and of course documentation).

[JackStouffer]

i'll take this

[JackStouffer]

jmdavis has remarked on how busy he is lately. I suggest you take him off because I don't think he has much time to even respond to datetime changes.

You can put me here instead

[jmdavis]

I have been very busy, and I do need to find more time for Phobos reviews, but on the whole, I don't want folks changing std.datetime without me at least seeing what they're doing, and I definitely don't want API changes being merged without me looking at them.

[JackStouffer]

@jmdavis Right, I was just saying you shouldn't be asked to review all changes to std.range.

[jmdavis]

Ah, okay. That's fine. I already get e-mails for every PR creation and comment anyway. So, if anything, the problem is not having key things buried rather than not being notified. Fortunately, I recently got it fixed so that my e-mail client is actually able to single out the e-mails that include @jmdavis, since my filter for that wasn't working before. And without a working filter for that sort of thing, I suspect that a number of us risk never seeing a message targeted at us (for anyone who cares and has also had filtering problems, the key thing for my e-mail client was to watch for the e-mail to be cc-ed to mention@noreply.github.com - since for whatever stupid reason, it was failing to filter based on the e-mail's content).

[JackStouffer]

I'll take array.d and ascii.d as well

[Biotronic]

I'd be happy to help out on std.meta, std.typecons, std.bigint, std.traits, std.math and std.mathspecial (in order of preference, if we're trying to limit numbers). I could also do std.range and std.algorithm, but I think that's nearing the limit of how much I should do.

[wilzbach]

@Biotronic wow - thanks a lot

(in order of preference, if we're trying to limit numbers).

Nope there's no limit - on the contrary redundancy is good!
Maybe GitHub's word explain this a bit:

While effective code review is essential to every successful project, it's not always clear who should review files—even with GitHub's reviewer suggestions.

So that's what this file is about: building a quicklist of qualified reviewers for Phobos modules - super-powered with GitHub features.

[MetaLang]

I can help with std.meta/typecons

[JackStouffer]

That covers all of the modules which change more than once a release (or once a year). I'd say this is ready to merge.

[wilzbach]

I'd say this is ready to merge.

I would wait for a bit (e.g. two or three days) to give people enough time to respond - especially since we haven't gotten the OK from many people on this file.

[kyllingstad]

Feel free to add my name to std.complex and std.path too. I rewrote both of them some years ago and feel a certain sense of ownership even if I'm not as active as I used to be.

[PetarKirov]

You can add me here as well. Though, I guess @schveiguy would want to be at the front of the list?

[PetarKirov]

+1 for std/algorithm and std/array

[PetarKirov]

+1 for std/experimental/allocator

[PetarKirov]

+1 for std/range

[PetarKirov]

+1 for std/typecons.d

[WebDrake]

Just a small technical point here.

IIUC the point of CODEOWNERS is that it blocks a PR from being mergeable unless an owner has signed off on the change. That's different from a list of people from whom review is automatically requested.

Are we clear about the impact of introducing this? I'm good with the idea of being automatically notified about PRs for std.random, but I'm not good with the idea of being a bottleneck for them.

[wilzbach]

IIUC the point of CODEOWNERS is that it blocks a PR from being mergeable unless an owner has signed off on the change. That's different from a list of people from whom review is automatically requested.

To quote GitHub:

Code owners are automatically requested for review when someone opens a pull request that modifies code that they own. When someone with admin permissions has enabled required reviews, they can optionally require approval from a code owner.

The approval isn't enabled.

I'm good with the idea of being automatically notified about PRs for std.random

Cool!

I'm not good with the idea of being a bottleneck for them.

FWIW in particular regarding std.random I wouldn't worry about being the bottleneck 😉

[WebDrake]

@wilzbach all good, then. I'm glad to see this feature being used so quickly :-)

[MartinNowak]

How did you produce the mapping? I'd be a little more comfortable with the automated recommended reviewer mechanism which seems to work fairly well and takes into account recent edits (and thus trends). At least we should figure out how to regularly update that mapping.

[jpf91]

Sounds like a great new feature. Now if we only could subscribe to all new bugzilla issues filed for a certain module...

std/digest/* @jpf191

OK. But there's a typo here, my user handle is jpf91 ;-)
I can also take care of std.uuid as I initially wrote that module. (and it's a trivial one anyway ;-)

And if you add CODEOWNERS for druntime as well I can adopt the fiber code core/threadasm.S. I'm somewhat familiar with the fiber implementation as I wrote the ARM implementation and some of the docs.

[CyberShadow]

Now if we only could subscribe to all new bugzilla issues filed for a certain module...

I think that's certainly possible. Bugzilla has components, and it's possible to assign owners to components (currently all components have no owner).

@braddr Do you think it would be possible to configure Bugzilla so that each Phobos module can be assigned an owner?

[burner]

how was this list generated? bit-blame?

I would wait for a bit (e.g. two or three days) to give people enough time to respond -

IMHO two or three days is a way to short timeline. I think two, three weeks is much more realistic. Especially, for an org change this big.

I also would like to be an owner of std.string.

[JackStouffer]

@burner There's no org change. In essence, this is an automated pinging system. See @MartinNowak's earlier comments.

[burner]

this is an automated pinging system

sounds like a org. change to me

Anyway, how did you generate the list?

[wilzbach]

how was this list generated? bit-blame?

Manually by looking at the the "Authors" section of the most popular modules (filtering out "dead" people, of course). I did this to get an initial list on which can be improved, which has already heavily happened in this PR ;-)

OK. But there's a typo here, my user handle is jpf91 ;-)

Oh I am terribly sorry. Fixed.

[burner]

IFS=$'\n'
for i in `git shortlog -s $1  | cut -c8-`
do
    echo $i
    git blame $1 | grep -c $i
done

gitstats is properly the better idea then this small script

[wilzbach]

gitstats is properly the better idea then this small script

Good idea: http://files.wilzbach.me/dlang/gitstats/

Btw I also provide something more rough as a handy API for dlang.org: https://contribs.dlang.io/contributors/file/dlang/phobos?file=std/ascii.d (it's used to display the contributors list at the bottom of each module, here: https://dlang.org/phobos/std_ascii.html)

However, as you are interested I quickly hacked together another script:

ls std | sort | parallel -j1 "printf '\n\n> {}\n' && python2 ./git-score -o commits std/{} | sed 's/<.*@.*>/<mail@obscured.com>/'" > per_module_stats

-> results here - the results are quite ugly (didn't have time to remove the coloring) and didn't filter for maintenance commits.

[burner]

replace "ls std" with "find std -name *.d" that should also make sub directories work

[wilzbach]

replace "ls std" with "find std -name *.d" that should also make sub directories work

I used ls on purpose as for most files (except std.experimental) the module is more important (e.g. std.algorithm, std.container, std.datetime, std.digest, std.range, std.regex, std.windows ...)

Anyway - before we waste more time here. Can we get sth. actionable from these stats? Many people on the git history have switched their focus to other areas and it wouldn't make sense to ping them on new PRs.
-> Suggestions welcome.

[CyberShadow]

Are there any people in the file who haven't been active lately or haven't replied here? E.g. I haven't seen @don-clugston-sociomantic on GitHub in a while.

[wilzbach]

Are there any people in the file who haven't been active lately or haven't replied here? E.g. I haven't seen @don-clugston-sociomantic on GitHub in a while.

That's why I wanted to wait a bit - though I think only responses from @don-clugston-sociomantic, @schveiguy and @DmitryOlshansky are outstanding. We could remove all (or only Don) for now, squash the commits and go ahead with the initial version?
(I am pretty sure this file will evolve over time anyhow.)

[jmdavis]

As I understand it, Don has gotten far busier in recent years (primarily due to spending time with his family, I think), and he's really not very active with D at this point outside of work. So, having him on the list doesn't really make sense even if it would be nice to have him around more. Steven and Dmitry are certainly active here on github, but I don't know how they feel about this. They probably already get e-mails about all of the Phobos PRs.

[wilzbach]

Don has gotten far busier in recent years (primarily due to spending time with his family, I think), and he's really not very active with D at this point outside of work.

Thanks for the info -> removed Don (also squashed the file and sorted the owners within one line alphabetically).

[schveiguy]

I'm fine with being the "owner" of some modules in Phobos, whatever that means. The only module I truly am responsible for is std.container.rbtree. I don't mind seeing std.process PRs as well.

I'll have to see if it makes any difference in the notifications. I have been swamped lately at work and have almost 7k unread messages in my github email folder. Anything that helps me filter which messages are more likely to concern me would be useful.

[jmdavis]

I'll have to see if it makes any difference in the notifications. I have been swamped lately at work and have almost 7k unread messages in my github email folder. Anything that helps me filter which messages are more likely to concern me would be useful.

It wouldn't surprise me if it has an extra CC address that you can filter on, since they do that with @ mentions.

[MartinNowak]

Good idea: http://files.wilzbach.me/dlang/gitstats/

Jonathan and me seem to be the only phobos contributors with a net negative line contribution in that list.
Make sure to obey mailmap for correct attribution https://github.com/dlang/dmd/blob/master/.mailmap.

[MartinNowak]

Wrote a small script to analyse git blames @wilzbach.
Get top contributors for a source file of a git repo.
Seem to work pretty well FWIW.

dmd_contributors.txt
druntime_contributors.txt
phobos_contributors.txt

[jmdavis]

Jonathan and me seem to be the only phobos contributors with a net negative line contribution in that list.

Seb's page there doesn't think that I've committed since 2014, so I suspect that it assumes that someone has only ever have one e-mail address.

[wilzbach]

Wrote a small script to analyse git blames

Nice! For the orphaned modules, I have posted the list of the top5 with GH mapping to the Phobos slack channel (ping Martin or me if you need an invite). I can post them here as well, but I wasn't sure whether it's a good idea to ping so many people.

That covers all of the modules which change more than once a release (or once a year). I'd say this is ready to merge.

Yep, I think this list will evolve over time anyways. If someone thinks it's a good idea to ping nearly all contributors of the orphaned modules, I am happy to open a new PR ;-)

@ future readers: If you have in-depth knowledge of a Phobos module, please don't hesitate to add yourself to the respective list (even if already contains contributors).

[DmitryOlshansky]

Nice! For the orphaned modules, I have posted the list of the top5 with GH mapping to the Phobos slack channel (ping Martin or me if you need an invite). I can post them here as well, but I wasn't sure whether it's a good idea to ping so many people.

@wilzbach Invite me ;)