Bug 215960

<rdar://problem/67968919>

Created attachment 407517 [details] Patch

Created attachment 407608 [details] Patch

Comment on attachment 407608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407608&action=review > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:42 > + def root(self): Not sure if "root" is the right name, this is returning top level directory path of a working tree (git) or a working copy (svn), maybe top_level_path / root_path ? Besides do we consider to make this an argument ? > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:50 > + result = run([self.executable, 'rev-parse', '--abbrev-ref', 'HEAD'], cwd=self.root, capture_output=True, encoding='utf-8') This won't work in detached head state, for example you directly checkout with commit hash, we need to detect this state, otherwise it will return "HEAD" instead of an empty string, "git branch --show-current" will return an empty string, and should make this method failed quickly > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:33 > + self.remote = remote or 'git@github.org:/mock/{}'.format(os.path.basename(path)) maybe we shouldn't use GitHub in the mock path

Comment on attachment 407608 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407608&action=review >> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:42 >> + def root(self): > > Not sure if "root" is the right name, this is returning top level directory path of a working tree (git) or a working copy (svn), maybe top_level_path / root_path ? > Besides do we consider to make this an argument ? The point of this is so that if a user constructs a repository at a location, we know what the root of the working tree is (basically, making it so that "Git(os.path.dirname(__file__))" works.

Created attachment 407630 [details] Patch

Comment on attachment 407630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407630&action=review > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:32 > + self.cached = cached What's the purpose of this variable? Why do we want a memoizer that does not cache result? > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:37 > + timeout = kwargs.get('timeout', self.timeout) > + cached = kwargs.get('cached', self.cached) What happens if the wrapped function actually uses `timeout` and `cached` for different purpose? > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:48 > + cache = function(*args) Also, I think we should pass '**kwargs' here? > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:53 > + decorator.clear = lambda: self.clear() Why not just decorator.clear = self.clear?

Comment on attachment 407630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407630&action=review >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:32 >> + self.cached = cached > > What's the purpose of this variable? Why do we want a memoizer that does not cache result? We may want a memoizer which does not cache by default. That would allow you to have a function call which explicitly opted in to the cached behavior. It's basically the reverse of what I would imagine most usages of the class will be: default caching, except for some calls which opt out of caching. We don't have to keep this, it just seemed silly not to given how decorator was implemented. >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:37 >> + cached = kwargs.get('cached', self.cached) > > What happens if the wrapped function actually uses `timeout` and `cached` for different purpose? We would be passing those arguments in both spots. Suppose we should probably pop them instead, so they don't get forwarded. >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:48 >> + cache = function(*args) > > Also, I think we should pass '**kwargs' here? Oops, yes!

Created attachment 407833 [details] Patch

Comment on attachment 407833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407833&action=review > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:46 > + if cache: > + return cache Does this mean a function that returns None will always not cached even when 'None' is a valid return of the function? We may want to differentiate that in the code. > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/tests/decorators_unittest.py:31 > + It would be great to test a function that throws an exception and the exception should be raised as expected.

Comment on attachment 407833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407833&action=review >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:46 >> + return cache > > Does this mean a function that returns None will always not cached even when 'None' is a valid return of the function? > We may want to differentiate that in the code. Oops! Yes it does

Created attachment 407923 [details] Patch

Created attachment 408007 [details] Patch for landing

Committed r266628: <https://trac.webkit.org/changeset/266628> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408007 [details].