Bug 235161 – [EWS] Load contributors from stand-alone class

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

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.

[Radar WebKit Bug Importer]

<rdar://problem/87491516>

[Jonathan Bedard]

Pull-request: https://github.com/WebKit/WebKit/pull/70

[Jonathan Bedard]

(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

[Jonathan Bedard]

Created attachment 449007 [details]
Patch

[Jonathan Bedard]

Created attachment 449011 [details]
Patch

[Aakash Jain]

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)

[Jonathan Bedard]

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

[Jonathan Bedard]

Created attachment 449056 [details]
Patch

[Jonathan Bedard]

Landed 246007@main (r287978)

[Aakash Jain]

(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.