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 #5629

Merged
merged 10 commits into from
Dec 8, 2023
Merged

add CODEOWNERS file #5629

merged 10 commits into from
Dec 8, 2023

Conversation

tdhock
Copy link
Member

@tdhock tdhock commented Apr 26, 2023

The CODEOWNERS file is documented here, https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners
Basically, github uses this file to automatically notify people for code review, when a (non-draft) PR is opened.
Creating this explicit mapping from files to reviewers would be essential to enable a more de-centralized code review process (less work for @mattdowle).
In this first draft of CODEOWNERS, I have volunteered to be reviewer for files related to melt.
@MichaelChirico @jangorecki @ben-schwen would you please (1) add lines to CODEOWNERS to indicate files that you would like to review, and (2) @tag-other-people who you think may be interested to volunteer as reviewers?

@tdhock
Copy link
Member Author

tdhock commented May 4, 2023

In this PR let's also do some re-organization of what objects are defined in which R/* files, creating new smaller files with more specific functionality. For example the new/proposed DT function is currently defined in data.table.R, which has a lot of other things in it too, so maybe we could move the DT defintion to its own file DT.R?

@jangorecki
Copy link
Member

jangorecki commented May 4, 2023

No reorganizing, too many conflicts (in other pending PRs) will be there.

@jangorecki
Copy link
Member

Is it still a draft? IMO it's ready to merge as is

@tdhock
Copy link
Member Author

tdhock commented May 8, 2023

maybe wait for @MichaelChirico ?

@MichaelChirico
Copy link
Member

Hi folks, just back from vacation.

Question about this, I was thinking of adding some files that I have a relatively high amount of experience with (e.g. fread), but I wasn't clear about the syntax after reading the docs.

If we have

* @mattdowle
/src/fread.c @michaelchirico

Does that mean Matt's "ownership" is overridden by mine for src/fread.c?

I think that means we should use

* @mattdowle
/src/fread.c @mattdowle @michaelchirico

To ensure Matt can still be an owner for that file, is that right?

@MichaelChirico
Copy link
Member

MichaelChirico commented May 8, 2023

I was also unsure my most highly-contributed parts of the repo so I did the following (in case it's useful to anyone else):

stats = 'git log --author="michaelchirico4@gmail.com" --pretty=tformat: --numstat' |>
  system(intern = TRUE) |>
  paste(collapse = "\n") |>
  data.table::fread(na.strings = "-") |>
  data.table::setnames(c("added", "removed", "path"))
stats = stats[, by = path, .(
  added = sum(as.integer(added), na.rm = TRUE),
  removed = sum(as.integer(removed), na.rm = TRUE)
)]
stats[, total := added + removed]
stats[order(-total)]

@ben-schwen
Copy link
Member

Question about this, I was thinking of adding some files that I have a relatively high amount of experience with (e.g. fread), but I wasn't clear about the syntax after reading the docs.

If we have

* @mattdowle
/src/fread.c @michaelchirico

Does that mean Matt's "ownership" is overridden by mine for src/fread.c?

I think that means we should use

* @mattdowle
/src/fread.c @mattdowle @michaelchirico

To ensure Matt can still be an owner for that file, is that right?

Good point. Yes, only the last match for a file counts, according to docs. So if we want Matt to be the code owner of every file we need to add him to every line. However, I thought the whole idea of this is to take smth off Matt's plate.

@jangorecki
Copy link
Member

jangorecki commented Aug 30, 2023

Once we have interested contributors gathered in this PR, then we could consider some, ideally minor, file structure reorganizations. So it could possibly be easier to assign owners of files. For example R/data.table.R holds way too much stuff. If some parts of it would be under ownership of a particular contributor, then could make sense to take that parts out into new file. Nevertheless we should do this in very conservative way, as it disturbs git history browsing. And do it after merging pending PRs, otherwise it will be nightmare to resolve conflicts.

Copy link
Member

@jangorecki jangorecki left a comment

Choose a reason for hiding this comment

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

LGTM

@jangorecki jangorecki added this to the 1.15.0 milestone Dec 8, 2023
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9225c16) 97.46% compared to head (4fd5119) 97.46%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5629   +/-   ##
=======================================
  Coverage   97.46%   97.46%           
=======================================
  Files          80       80           
  Lines       14814    14814           
=======================================
  Hits        14439    14439           
  Misses        375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jangorecki jangorecki merged commit 93296be into master Dec 8, 2023
5 checks passed
@jangorecki jangorecki deleted the CODEOWNERS branch December 8, 2023 15:12
@tdhock
Copy link
Member Author

tdhock commented Dec 8, 2023

thanks!

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

Successfully merging this pull request may close these issues.

4 participants