Bug 235161 - [EWS] Load contributors from stand-alone class
Summary: [EWS] Load contributors from stand-alone class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-12 15:39 PST by Jonathan Bedard
Modified: 2022-01-14 04:08 PST (History)
2 users (show)

See Also:


Attachments
Patch (10.12 KB, patch)
2022-01-12 15:49 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (10.03 KB, patch)
2022-01-12 15:59 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.98 KB, patch)
2022-01-13 06:37 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2022-01-12 15:39:08 PST
Loading contributors.json is something other steps may also with to do (namely, ValidateChange). We should break off contributors.json parsing into it's own class.
Comment 1 Radar WebKit Bug Importer 2022-01-12 15:39:27 PST
<rdar://problem/87491516>
Comment 2 Jonathan Bedard 2022-01-12 15:40:03 PST
Pull-request: https://github.com/WebKit/WebKit/pull/70
Comment 3 Jonathan Bedard 2022-01-12 15:48:42 PST
(In reply to Jonathan Bedard from comment #2)
> Pull-request: https://github.com/WebKit/WebKit/pull/70

Posted on the wrong bug, https://github.com/WebKit/WebKit/pull/71 is actually the PR for this bug
Comment 4 Jonathan Bedard 2022-01-12 15:49:34 PST
Created attachment 449007 [details]
Patch
Comment 5 Jonathan Bedard 2022-01-12 15:59:14 PST
Created attachment 449011 [details]
Patch
Comment 6 Aakash Jain 2022-01-12 17:39:47 PST
Comment on attachment 449011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449011&action=review

This re-organization seems fine, just need to be careful in subsequent patches how exactly we use it, especially what we display to our users. Also left few comments.

> Tools/CISupport/ews-build/steps.py:139
> +                    printable='{} <{}>'.format(name, emails[0].lower()),

let's skip this printable from this patch, and add it in subsequent patch when we implement the exact use-case.

> Tools/CISupport/ews-build/steps.py:141
> +                    email=emails[0],

Do we need to store name here?

> Tools/CISupport/ews-build/steps.py:1016
> +    def __init__(self, *args, **kwargs):

Let's skip init and just have 'contributors = {}' as a class variable (as it was before)
Comment 7 Jonathan Bedard 2022-01-13 06:29:41 PST
Comment on attachment 449011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449011&action=review

>> Tools/CISupport/ews-build/steps.py:139
>> +                    printable='{} <{}>'.format(name, emails[0].lower()),
> 
> let's skip this printable from this patch, and add it in subsequent patch when we implement the exact use-case.

Sounds good!

>> Tools/CISupport/ews-build/steps.py:141
>> +                    email=emails[0],
> 
> Do we need to store name here?

The other mapping is from email -> name/status. We're going to need username -> email/name/status
Comment 8 Jonathan Bedard 2022-01-13 06:37:33 PST
Created attachment 449056 [details]
Patch
Comment 9 Jonathan Bedard 2022-01-13 07:24:06 PST
Landed 246007@main (r287978)
Comment 10 Aakash Jain 2022-01-14 04:08:32 PST
(In reply to Jonathan Bedard from comment #9)
> Landed 246007@main (r287978)

+  email=emails[0],
Please check if we should use emails[0].lower() similar to how we use for bugzilla email. There are some emails in contributors.json which use mixed case.