[webkit-reviews] review granted: [Bug 193735] [ews-app] Add method to save Build data to database : [Attachment 359931] Proposed path

[bugzilla-daemon at webkit.org]

Lucas Forschler <lforschler at apple.com> has granted Aakash Jain
<aakash_jain at apple.com>'s request for review:
Bug 193735: [ews-app] Add method to save Build data to database
https://bugs.webkit.org/show_bug.cgi?id=193735

Attachment 359931: Proposed path

https://bugs.webkit.org/attachment.cgi?id=359931&action=review




--- Comment #2 from Lucas Forschler <lforschler at apple.com> ---
Comment on attachment 359931
  --> https://bugs.webkit.org/attachment.cgi?id=359931
Proposed path

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

logic looks good to me, with possible name/refactoring.

> Tools/BuildSlaveSupport/ews-app/ews/common/util.py:46
> +def is_valid_int_id(id):

would this be ok as 'is_valid_id' ?

> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:51
> +    def save_build(cls, patch_id, buildid, builderid, number, result,
state_string, started_at, complete_at=None):

since we are using underscores for patch_id, should we continue that with
build_id, builder_id?

> Tools/BuildSlaveSupport/ews-app/ews/models/build.py:72
> +	   if not (util.is_valid_int_id(patch_id) and
util.is_valid_int_id(buildid) and util.is_valid_int_id(builderid)  and
util.is_valid_int_id(number)):

nit: extra space before the last 'and'