Bug 191931 – [ews-app] Add methods to update Patch fields

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Aakash Jain]

Add methods to update Patch fields like set_obsolete, set_bug_id etc. These methods would help if someone wants to update a specific field in Patch. These methods would do necessary verification.

[Aakash Jain]

Created attachment 355548 [details]
Proposed patch

Part of patch series. Therefore wouldn't apply to ToT without applying other patches first.

[Json]

Comment on attachment 355548 [details]
Proposed patch

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

> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:78
> +    def set_sent_to_bb(cls, patchid, sent=True):

I'm not sure if I like using 'bb' as an abbreviation for buildbot... it's not super clear for people who are new to the code/lingo.
would it be terrible to use set_sent_to_buldbot ?

Also... 'set' could be read multiple ways....
set: this is a set of cards
set: set variable x to foo

it wasn't obvious to me which usage this was, just by looking at the function name.

> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:83
> +       if patch.sent_to_bb == sent:

could sent_to_bb be a boolean?

[Kocsen Chung]

Comment on attachment 355548 [details]
Proposed patch

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

>> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:78
>> +    def set_sent_to_bb(cls, patchid, sent=True):
> 
> I'm not sure if I like using 'bb' as an abbreviation for buildbot... it's not super clear for people who are new to the code/lingo.
> would it be terrible to use set_sent_to_buldbot ?
> 
> Also... 'set' could be read multiple ways....
> set: this is a set of cards
> set: set variable x to foo
> 
> it wasn't obvious to me which usage this was, just by looking at the function name.

I agree, we should be explicit about as many things as we can.

Ditto with patchid => patch_id.

This is kind of orthogonal to this patch but another thing to keep in mind is to avoid redundant naming within a model. It a Patch has an id it should be Patch.id instead of Patch.patch_id etc.

[Aakash Jain]

Created attachment 355891 [details]
Updated patch

> I'm not sure if I like using 'bb' as an abbreviation for buildbot.
Expanded bb to buildbot.

> Also... 'set' could be read multiple ways.
Not sure what might be a good alternative. mark_set_to_buildbot or set_property_sent_to_buildbot doesn't look good as well. Kept as is for now.

> could sent_to_bb be a boolean?
It is a boolean.

[EWS Watchlist]

Attachment 355891 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:79:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:80:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:82:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:83:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:84:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:85:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:87:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:88:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:89:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:90:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:94:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:95:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:97:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:98:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:98:  multiple spaces before operator  [pep8/E221] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:99:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:100:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:102:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:103:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:104:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:105:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:109:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:110:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:112:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:113:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:114:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:115:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:116:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:117:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:118:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:119:  indentation is not a multiple of four  [pep8/E111] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:80:  [Patch.set_sent_to_buildbot] Undefined variable 'ERR_NON_EXISTING_PATCH'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:82:  [Patch.set_sent_to_buildbot] Class 'Patch' has no 'objects' member  [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:85:  [Patch.set_sent_to_buildbot] Undefined variable 'SUCCESS'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:90:  [Patch.set_sent_to_buildbot] Undefined variable 'SUCCESS'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:95:  [Patch.set_bug_id] Undefined variable 'ERR_NON_EXISTING_PATCH'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:97:  [Patch.set_bug_id] Class 'Patch' has no 'objects' member  [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:100:  [Patch.set_bug_id] Undefined variable 'SUCCESS'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:105:  [Patch.set_bug_id] Undefined variable 'SUCCESS'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:110:  [Patch.set_obsolete] Undefined variable 'ERR_NON_EXISTING_PATCH'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:112:  [Patch.set_obsolete] Class 'Patch' has no 'objects' member  [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:115:  [Patch.set_obsolete] Undefined variable 'SUCCESS'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:119:  [Patch.set_obsolete] Undefined variable 'SUCCESS'  [pylint/E0602] [5]
Total errors found: 43 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aakash Jain]

Created attachment 355893 [details]
Updated patch

[EWS Watchlist]

Attachment 355893 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:82:  [Patch.set_sent_to_buildbot] Class 'Patch' has no 'objects' member  [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:97:  [Patch.set_bug_id] Class 'Patch' has no 'objects' member  [pylint/E1101] [5]
ERROR: Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:112:  [Patch.set_obsolete] Class 'Patch' has no 'objects' member  [pylint/E1101] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lucas Forschler]

Comment on attachment 355891 [details]
Updated patch

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

> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:83
> +       if patch.sent_to_buildbot == sent:

since the value of 'sent' is True or False, can we reduce this to:

if patch.sent_to_buildbot:

> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:87
> +       patch.sent_to_buildbot = sent

We chatted about never setting sent=False...  if that is something we won't do, then it feels better to me to write this as:
patch.sent_to_buildbot = True

this removes any doubt that the value is being set to True.

> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:113
> +       if patch.obsolete == obsolete:

can we simplify this also?
if patch.obsolete:

> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:116
> +       patch.obsolete = obsolete

We discussed if someone flips the state of the 'obsolete' flag inadvertently, or on purpose... so we need to make sure we handle the following cases:
set obsolete = True when obsolete flag is already true (no op) (warning?)
set obsolete = True when obsolete flag is false
set obsolete = False when obsolete flag is already false (no op) (warning?)
set obsolete = False when obsolete flag is true.

Maybe the best way to do this is to simply change the _log.warn to:
_log.warn('Patch {} is already marked with obsolete={}.'.format(patch_id, obsolete))

[Lucas Forschler]

Comment on attachment 355891 [details]
Updated patch

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

>> Tools/BuildSlaveSupport/ews-app/ews/models/patch.py:113
>> +       if patch.obsolete == obsolete:
> 
> can we simplify this also?
> if patch.obsolete:

(if we take the below approach, this should stay as written)

[Aakash Jain]

Incorporated above feedback.

Committed r238627: <http://trac.webkit.org/changeset/238627>

[Radar WebKit Bug Importer]

<rdar://problem/46317844>