Summary: | [webkitcorepy] Add API to efficiently create a sequence of commits | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Bedard <jbedard> | ||||||||||||||
Component: | Tools / Tests | Assignee: | Jonathan Bedard <jbedard> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | aakash_jain, dean_johnson, dewei_zhu, slewis, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Jonathan Bedard
2021-04-21 13:13:35 PDT
Created attachment 426736 [details]
Patch
Created attachment 427192 [details]
Patch
Created attachment 427234 [details]
Patch
Created attachment 427702 [details]
Patch
Comment on attachment 427702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427702&action=review I like the changes so far -- the code definitely feels easier to read through. Left a few minor comments, but overall LGTM. Unofficial r+. > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/scm_base.py:79 > + for arg, value in kwargs.items(): Can you give some example arguments we expect to end up in kwargs here? Is it possible for us to pass arguments directly called "begin" and "end" instead? I suspect including new data in **kwargs could end up having an unintended effect in the future. > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:379 > + if commit.hash == previous[0].hash: Could you add some comments for these if/elif/else blocks to help a reader better understand what's happening here, and why? > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/svn.py:417 > + return dict( Is it possible to refactor the Git function to return a similarly structured dictionary (defined in this way)? This function makes it really easy to understand what data we expect to be returned. > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/svn.py:450 > + if line and line.rstrip() != '-' * 72: This might be premature optimization, but maybe we should consider defining a variable / regex for `'-' * 72`? Currently it's producing that string on every line read. > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:497 > + for branch, commits in self.commits.items(): It seems like when we're looking for `commits_in_range` / on the default branch we could avoid running this loop entirely. If we plan on changing this code in the future to make use of a hash => identifier lookup table, you can ignore this comment (but it would be good to mention it in the code). > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:519 > + if previous and branch != self.default_branch: Nit: It might be cleaner to write this as an early return, rather than nesting the if block: ``` if not previous or branch = self.default_branch: continue ``` Comment on attachment 427702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427702&action=review >> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/scm_base.py:79 >> + for arg, value in kwargs.items(): > > Can you give some example arguments we expect to end up in kwargs here? Is it possible for us to pass arguments directly called "begin" and "end" instead? I suspect including new data in **kwargs could end up having an unintended effect in the future. I like the point about "begin" and "end" dictionaries, will change >> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:497 >> + for branch, commits in self.commits.items(): > > It seems like when we're looking for `commits_in_range` / on the default branch we could avoid running this loop entirely. If we plan on changing this code in the future to make use of a hash => identifier lookup table, you can ignore this comment (but it would be good to mention it in the code). This is a helper function for our mock code. It's only ever gathering from an in-memory dictionary, and turning that dictionary into the output of various git commands so we can test against them. Created attachment 427713 [details]
Patch
rs=me Created attachment 427905 [details]
Patch for landing
Committed r277102 (237404@main): <https://commits.webkit.org/237404@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427905 [details]. |