contributors.json should be relocated outside of webkitpy, because it's applicable to the entire repository, and used by tooling, not part of tooling.
<rdar://problem/82552403>
Created attachment 436826 [details] Patch
Pull-request: https://github.com/WebKit/WebKit/pull/6
Can we split off the inactive contributors from the top-level JSON while we're at it?
I also wonder if yaml would be a better fit for this file anyway. JSON is just so verbose!
JSON is easier to parse in JavaScript, so probably not (bugs.webkit.org does this). Not quite sure how to best identify inactive contributors programmatically, but yes, would be nice to have fewer autocomplete suggestions in CC lists.
We can also consider changing the formatting to make it compact, similar to https://commits.webkit.org/240315@main (r280725)
(In reply to Aakash Jain from comment #7) > We can also consider changing the formatting to make it compact, similar to > https://commits.webkit.org/240315@main (r280725) Compacting contributors.json is a bit more difficult than you might think, because we have a script that re-writes contributors.json with pretty-print.
Created attachment 436901 [details] Part 1
Created attachment 436904 [details] Part 2
Created attachment 436907 [details] Part 2
Created attachment 436908 [details] Part 3
Created attachment 436912 [details] Part 4
Created attachment 436914 [details] Part 3
Created attachment 436921 [details] Part 4
Created attachment 436925 [details] Part 5
Comment on attachment 436901 [details] Part 1 View in context: https://bugs.webkit.org/attachment.cgi?id=436901&action=review > ChangeLog:9 > + * metadata/contributors.json: Copied from Tools/Scripts/webkitpy/common/config/contributors.json, made into list. As I mentioned on slack, I don't think this is a good idea to modify the file and move it in single commit. As a reviewer, it's extremely hard for me to verify whether there were any mistakes while copying and editing this large file. There should be one commit which just move/copy the file without any changes in contributors.json at all (and required changes in other files), so that there isn't need to review the contents of contributors.json. There should be separate commit to change the contents of contributors.json
Comment on attachment 436901 [details] Part 1 View in context: https://bugs.webkit.org/attachment.cgi?id=436901&action=review >> ChangeLog:9 >> + * metadata/contributors.json: Copied from Tools/Scripts/webkitpy/common/config/contributors.json, made into list. > > As I mentioned on slack, I don't think this is a good idea to modify the file and move it in single commit. As a reviewer, it's extremely hard for me to verify whether there were any mistakes while copying and editing this large file. > > There should be one commit which just move/copy the file without any changes in contributors.json at all (and required changes in other files), so that there isn't need to review the contents of contributors.json. There should be separate commit to change the contents of contributors.json I understand the sentiment (although the file was generated by validate-committer-lists, as happens when a user modifies contributors.json normally, not manually edited), but the mechanics of editing a file like contributors.json, which is accessed by services that aren't version locked, means that we need a way to determine if we're looking at a new or old version of contributors.json. The way that webkit.org, bugs.webkit.org, ewe-build.webkit.org and webkit-bot all access contributors.json is directly from it's tip-of-tree source via either svn.webkit.org or GitHub. If we don't tie the format change of contributors.json to the rename of the file, these services can't tell if they're dealing with the new file format, or the old one
Aakash and I talked about this via slack, our rough plan is this: 1. Copy existing contributors.json 2. Refactor contributors.json 3-6. Adopt new contributors.json in services
Created attachment 436953 [details] Patch
Comment on attachment 436953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436953&action=review > ChangeLog:9 > + * metadata/contributors.json: Copied from Tools/Scripts/webkitpy/common/config/contributors.json. was it copied through svn cp?
Created attachment 436958 [details] Patch
Comment on attachment 436958 [details] Patch Sitting on this change until tomorrow
Committed r281850 (241182@main): <https://commits.webkit.org/241182@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436958 [details].
Reopening to attach new patch.
Created attachment 437031 [details] Patch
Since multiple different improvements are being made, I feel like the improvements should be done in separate bug(s) (with an explanation for that change). Making improvement to contributors.json with a title 'Relocate contributors.json' doesn't feel right.
(In reply to Aakash Jain from comment #27) > Since multiple different improvements are being made, I feel like the > improvements should be done in separate bug(s) (with an explanation for that > change). Making improvement to contributors.json with a title 'Relocate > contributors.json' doesn't feel right. I think this goes back to what we were discussing yesterday: the conversion from a dictionary to a list is part of the relocation when it comes to services. This is why I made a pull-request (https://github.com/WebKit/WebKit/pull/6) as well, so you can see why the changes to services need to combine the dictionary -> list change. If we try and separate the refactor of contributors.json, this is going to be nonsense. We have a sequence of changes which depend on one another, a patch series lets us clearly communicate that dependency chain.
Comment on attachment 437031 [details] Patch r=me
Committed r281883 (241213@main): <https://commits.webkit.org/241213@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437031 [details].
Created attachment 437078 [details] Part 3
Created attachment 437079 [details] Part 4
Created attachment 437081 [details] Part 5
Created attachment 437082 [details] Patch
Created attachment 437085 [details] Part 5
Created attachment 437086 [details] Part 6
Created attachment 437087 [details] Part 7
Created attachment 437157 [details] Part 3
Comment on attachment 437157 [details] Part 3 rs=me
Created attachment 437158 [details] Part 4
Created attachment 437159 [details] Part 5
Comment on attachment 437158 [details] Part 4 View in context: https://bugs.webkit.org/attachment.cgi?id=437158&action=review rs=me with a comment > Websites/bugs.webkit.org/committers-autocomplete.js:27 > + var COMMITTERS_URL = 'https://svn.webkit.org/repository/webkit/trunk/metadata/contributors.json'; we should start reading it from github (like ews). from https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors.json
(In reply to Aakash Jain from comment #43) > Comment on attachment 437158 [details] > Part 4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=437158&action=review > > rs=me with a comment > > > Websites/bugs.webkit.org/committers-autocomplete.js:27 > > + var COMMITTERS_URL = 'https://svn.webkit.org/repository/webkit/trunk/metadata/contributors.json'; > > we should start reading it from github (like ews). from > https://raw.githubusercontent.com/WebKit/WebKit/main/metadata/contributors. > json Agreed, but that seemed like something for a different change, this change is already more than we want it to be, I think
Comment on attachment 437159 [details] Part 5 View in context: https://bugs.webkit.org/attachment.cgi?id=437159&action=review > Tools/CISupport/ews-build/steps.py:907 > + repo_root = os.path.dirname(os.path.dirname(os.path.dirname(cwd))) It's fine for now, but this method of determining repo root based on location of current file seems fragile, would be good to calculate it in some other reliable method.
Committed r281932 (241241@main): <https://commits.webkit.org/241241@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437157 [details].
Still a few more changes to land.
Committed r281936 (241245@main): <https://commits.webkit.org/241245@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437158 [details].
Still more changes to land.
Committed r281938 (241247@main): <https://commits.webkit.org/241247@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437159 [details].
Created attachment 437192 [details] Part 6
Comment on attachment 437192 [details] Part 6 r=me Please change the running webkitbot code in the server.
Committed r281956 (241263@main): <https://commits.webkit.org/241263@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 437192 [details].
Created attachment 437222 [details] Part 7
Comment on attachment 437222 [details] Part 7 Last one!
Comment on attachment 437157 [details] Part 3 r=me
Comment on attachment 437222 [details] Part 7 r=me
Created attachment 440546 [details] Patch for landing
Committed r283750 (242672@main): <https://commits.webkit.org/242672@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440546 [details].