Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Add initital codeowners file#1870

Merged
andralex merged 1 commit intodlang:masterfrom
wilzbach:add-initial-codeowners
Dec 4, 2017
Merged

Add initital codeowners file#1870
andralex merged 1 commit intodlang:masterfrom
wilzbach:add-initial-codeowners

Conversation

@wilzbach
Copy link
Contributor

Similarly to dlang/phobos#5573

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.

I tried to use some insights from @MartinNowak's git blame analyzer, but this isn't perfect and your feedback and input would be very welcome: what are files in the druntime codebase that you feel comfortable with? Are there mappings that I added that don't reflect the status quo?

We had some troubles with GitHub at Phobos, but here's an example on how it will look in live.

@dlang/team-druntime @Abscissa @aG0aep6G @andralex @atilaneves @braddr @Burgos @Calrama @CyberShadow @Darredevil @deadalnix @DmitryOlshansky @etcimon @ibuclaw @JackStouffer @JacobCarlborg @jmdavis @joakim-noah @jpf91 @klickverbot @leandro-lucarella-sociomantic @MarcoLeisse @MartinNowak @mathias-lang-sociomantic @nrTQgc @rainers @redstar @schveiguy @tsbockman @WalterBright @wilzbach @ZombineDev and other awesome druntime contributors!

(I will wait a few days and if you don't confirm that you are ok with being pinged on a PR, I will remove you from the file for now).

@dlang-bot
Copy link
Contributor

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.

@Burgos
Copy link
Contributor

Burgos commented Jul 12, 2017

Hello and thanks for the initiative! Since I spent a bunch of time looking and debugging src/rt/sections_elf_shared.d, src/rt/dwarfeh.d, you can add me there as well.

@joakim-noah
Copy link
Contributor

Fine by me.

@tsbockman
Copy link
Contributor

Sure, you can list me for core.bitop if you want.

@MoritzMaxeiner
Copy link
Contributor

I'm okay with it.

@redstar
Copy link
Contributor

redstar commented Jul 13, 2017

Very nice. I'm ok with it.

@rainers
Copy link
Member

rainers commented Jul 13, 2017

Fine with me.

@schveiguy
Copy link
Member

These handles are incorrect, should be fixed: @mleise, @jacob-carlborg

Fine by me, thanks!

@jacob-carlborg
Copy link
Contributor

@wilzbach you can put me on src/rt/sections_osx*, src/rt/osx_tls.c and basically anything macOS or Apple related.

CODEOWNERS Outdated
src/core/simd.d @WalterBright @MartinNowak
src/core/stdc/* @schveiguy
src/core/stdcpp/* @WalterBright @Darredevil
src/core/sync/* @MartinNowak @mathias-lang-sociomantic @WalterBright
Copy link
Member

Choose a reason for hiding this comment

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

+1 for src/core/sync/*

CODEOWNERS Outdated
src/core/sys/posix/* @CyberShadow @MartinNowak @joakim-noah @redstar
src/core/sys/solaris/* @redstar
src/core/sys/windows/* @CyberShadow
src/core/thread.d @MartinNowak @ZombineDev @Burgos @jpf91
Copy link
Member

Choose a reason for hiding this comment

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

Can you move me at the end of the list? There much more knowledgeable people than me for this part of the codebase.

CODEOWNERS Outdated
src/core/demangle.d @WalterBright @MartinNowak @rainers @ibuclaw
src/core/exception.d @MartinNowak @WalterBright @jmdavis @CyberShadow
src/core/internal @MartinNowak @schveiguy
src/core/math.d @braddr @redstar
Copy link
Member

Choose a reason for hiding this comment

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

s/braddr/ibuclaw/

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok being a replacement for braddr.

@mleise
Copy link
Contributor

mleise commented Jul 13, 2017

Thanks @schveiguy for giving the correct handles. I didn't design any of the functionality of core.cpuid, but I am responsible for the GDC port. Looks fine to me.

CODEOWNERS Outdated
src/core/math.d @braddr @redstar
src/core/runtime.d @MartinNowak @Abscissa
src/core/simd.d @WalterBright @MartinNowak
src/core/stdc/* @schveiguy
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there should be at least two reviewers assigned per section? (I don't mind being pinged for any C binding-related PRs).

CODEOWNERS Outdated

src/checkedint.d @redstar @andralex @JackStouffer

src/core/atomic.d @WalterBright @aG0aep6G
Copy link
Contributor

Choose a reason for hiding this comment

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

I have an interest in core.atomic, but I'm far from being an expert on the subject, nor am I particularly familiar with the code. I made like two PRs for it.

CODEOWNERS Outdated

src/object.d @andralex @MartinNowak

src/rt/deh_win32.d @leandro-lucarella-sociomantic @klickverbot
Copy link
Contributor

@dnadlinger dnadlinger Jul 13, 2017

Choose a reason for hiding this comment

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

Should be @WalterBright, not me. This is the DMD-specific Win32 SEH implementation.

Choose a reason for hiding this comment

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

Me neither, I haven't use Windows in the past 20 years or more :)

@wilzbach wilzbach force-pushed the add-initial-codeowners branch from 8a785c9 to 2635b8e Compare December 1, 2017 15:31
@wilzbach
Copy link
Contributor Author

wilzbach commented Dec 1, 2017

Ok I finally found time to get back to this. Anyone still missing on this file or is it good to go as an initial version? (It can always be modified later)

@mathias-lang-sociomantic
Copy link
Contributor

Could you list my personal account (@Geod24 ) along my Sociomantic one ? Thanks.

@wilzbach wilzbach force-pushed the add-initial-codeowners branch from 2635b8e to b6a3110 Compare December 1, 2017 15:35
@wilzbach
Copy link
Contributor Author

wilzbach commented Dec 1, 2017

Could you list my personal account (@Geod24 ) along my Sociomantic one ? Thanks.

Sure. Done ;-)
I also changed it for the respective DMD PR: dlang/dmd#6990

CODEOWNERS Outdated

src/object.d @andralex @MartinNowak

src/rt/deh_win32.d @leandro-lucarella-sociomantic @klickverbot

Choose a reason for hiding this comment

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

Me neither, I haven't use Windows in the past 20 years or more :)

CODEOWNERS Outdated

src/object.d @andralex @MartinNowak

src/rt/deh_win32.d @WalterBright @leandro-lucarella-sociomantic

Choose a reason for hiding this comment

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

Please no, I don't know anything about this :)

Copy link
Member

Choose a reason for hiding this comment

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

Repeat after the pantomime - Oh yes he does! :)

CODEOWNERS Outdated
src/core/stdcpp/* @WalterBright @Darredevil
src/core/sync/* @MartinNowak @mathias-lang-sociomantic @Geod24 @WalterBright @ZombineDev
src/core/sys/bionic/* @joakim-noah
src/core/sys/darwin/* @klickverbot
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the names for src/core/sys/darwin/* and src/core/sys/osx/* should be the same.


src/core/atomic.d @WalterBright
src/core/attribute.d @jaoc-carlborg
src/core/bitop.d @schveiguy @tsbockman @mathias-lang-sociomantic @Geod24
Copy link
Member

Choose a reason for hiding this comment

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

@mathias-lang-sociomantic - did you ask for yourself added twice here? (Have I given away too much information by suggesting you have a second identity?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes he did:

Could you list my personal account (@Geod24 ) along my Sociomantic one ? Thanks.

So going undercover now would require a third account ;-)

CODEOWNERS Outdated
src/checkedint.d @redstar @andralex @JackStouffer

src/core/atomic.d @WalterBright
src/core/attribute.d @jaoc-carlborg
Copy link
Member

@ibuclaw ibuclaw Dec 3, 2017

Choose a reason for hiding this comment

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

Misspelt name here. You also misspelt it as jacob-carbord also.

CODEOWNERS Outdated
src/rt/sections_win* @rainers
src/rt/sections_osx* @jacob-carlbord
src/rt/osx_tls.c* @jacob-carlbord
src/rt/sections_elf_shared * @Burgos
Copy link
Member

Choose a reason for hiding this comment

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

Misplaced *

CODEOWNERS Outdated
src/core/atomic.d @WalterBright
src/core/attribute.d @jaoc-carlborg
src/core/bitop.d @schveiguy @tsbockman @mathias-lang-sociomantic @Geod24
src/core/cpuid.d @WalterBright @mleise @JackStouffer
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add myself here in place of @mleise? I assume that he shows up as he added GDC support.

CODEOWNERS Outdated
src/rt/sections_android.d @joakim-noah
src/rt/sections_win* @rainers
src/rt/sections_osx* @jacob-carlbord
src/rt/osx_tls.c* @jacob-carlbord
Copy link
Member

@ibuclaw ibuclaw Dec 3, 2017

Choose a reason for hiding this comment

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

I think this matcher is wrong (remove the *).

CODEOWNERS Outdated

src/checkedint.d @redstar @andralex @JackStouffer

src/core/atomic.d @WalterBright
Copy link
Member

Choose a reason for hiding this comment

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

You can add myself here as well.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 3, 2017

Made one last sweep, everything looks fine except for noted mistakes @wilzbach.

@wilzbach wilzbach force-pushed the add-initial-codeowners branch from b6a3110 to ac9f4a6 Compare December 3, 2017 17:24
@wilzbach
Copy link
Contributor Author

wilzbach commented Dec 3, 2017

Made one last sweep, everything looks fine except for noted mistakes @wilzbach.

Thanks a lot.

src/core/time.d @jmdavis @schveiguy @CyberShadow

src/etc* @deadalnix @MartinNowak
src/gc* @rainers @DmitryOlshansky @MartinNowak @leandro-lucarella-sociomantic
Copy link
Member

Choose a reason for hiding this comment

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

This will match subdirectories right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

I'll give it a tick, will give it a short while to let others give feedback.

@andralex
Copy link
Member

andralex commented Dec 4, 2017

I'll merge, we can change later as needed.

@andralex andralex merged commit 35300ba into dlang:master Dec 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.