Summary: | [results.webkit.org] Distinguish hash and revision | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | aakash_jain, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=221860 | ||||||||||
Attachments: |
|
Description
Jonathan Bedard
2021-02-12 16:52:08 PST
Created attachment 420196 [details]
Patch
Comment on attachment 420196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420196&action=review > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:-48 > - def __init__(self, repository_id, branch, id, timestamp=None, order=None, committer=None, message=None): can we keep it one line for readability. > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:55 > + ): We should probably have a check at the beginning of this method which limits passing only one of (id, revision, identifier). I believe passing multiple ones isn't expected. > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:76 > + self.revision = self.revision if self.revision else self.id Better to keep it explicit multi-line if else. When written that way, I don't see a reason to do: self.revision = self.revision > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:78 > + self.hash = self.hash if self.hash else self.id Ditto. > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:84 > + raise ValueError(f"'{self.revision}' is an revision") This error message doesn't look like an error, more like an information. Can be improved to look like an error and actually indicate what's the error here. Comment on attachment 420196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420196&action=review >> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:55 >> + ): > > We should probably have a check at the beginning of this method which limits passing only one of (id, revision, identifier). I believe passing multiple ones isn't expected. We will be passing multiple ones, as an example, https://commits.webkit.org/234094@main would be: id=234094@main hash= ee0111014bf107a0958f023114778b75eddc8a4d revision=272858 Created attachment 420324 [details]
Patch
Comment on attachment 420324 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420324&action=review > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:70 > + if re.match(r'[0-9]+$', self.id): Might be good to make this re a variable, like RE_SVN_REVISION > Tools/Scripts/libraries/resultsdbpy/resultsdbpy/controller/commit.py:73 > + elif re.match(r'[a-fA-F0-9]+$', self.id): Might be good to make this re a variable, like RE_GIT_HASH Created attachment 420333 [details]
Patch
Committed r272872: <https://commits.webkit.org/r272872> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420333 [details]. |