Summary: | [git-webkit] Add setup function | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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, ews-watchlist, glenn, 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=225991 https://bugs.webkit.org/show_bug.cgi?id=226024 https://bugs.webkit.org/show_bug.cgi?id=226980 https://bugs.webkit.org/show_bug.cgi?id=228597 https://bugs.webkit.org/show_bug.cgi?id=228867 |
||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 228867 | ||||||||||||||||||
Attachments: |
|
Description
Jonathan Bedard
2021-05-19 15:07:18 PDT
Created attachment 429107 [details]
Patch
Comment on attachment 429107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429107&action=review > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/decorators.py:68 > +class hybridmethod(object): Let's chat about this one on Monday. > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/terminal.py:44 > + return default or options[0] I don't think we should automatically choose any value other than the default, otherwise someone who provides an invalid answer to 'Yes' / 'No' would always get yes, even if they were required to enter something. We also may want to consider a 'required' option where this can loop if we fail to parse the input from the user. > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:56 > + if isinstance(context, type): Is this trying to figure out if there is an instance of the class, or the object, because of @decorators.hybridmethod? This seems excessively complex -- can we implement this function with less abstractions? > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/setup.py:42 > + def git(cls, args, repository, **kwargs): I wonder if this code should instead be on the repository object, so SVN / Git / SCM-X could implement their own setup functions? Comment on attachment 429107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429107&action=review >> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/terminal.py:44 >> + return default or options[0] > > I don't think we should automatically choose any value other than the default, otherwise someone who provides an invalid answer to 'Yes' / 'No' would always get yes, even if they were required to enter something. > > We also may want to consider a 'required' option where this can loop if we fail to parse the input from the user. Perhaps if no default is specified, then we loop until a matching answer is found? >> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:56 >> + if isinstance(context, type): > > Is this trying to figure out if there is an instance of the class, or the object, because of @decorators.hybridmethod? This seems excessively complex -- can we implement this function with less abstractions? The essential problem here is that there exists both a global git config and a local git config, and we need access to both. The current implementation (using the hybrid method), makes this distinction by treating Git.config() and Git().config() differently, because that is the essence of what `git config -l` does, behave differently when in the global context vs the local context. >> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/setup.py:42 >> + def git(cls, args, repository, **kwargs): > > I wonder if this code should instead be on the repository object, so SVN / Git / SCM-X could implement their own setup functions? I did think about this, but there really isn't a case for running the setup except through the command-line script. I didn't want to muddy the waters for other potentials clients of webkitscmpy, setup is a function that should only be preformed by a script explicitly called by the user. Created attachment 434874 [details]
Patch
Comment on attachment 434874 [details]
Patch
An initial crack at a setup function which configures your WebKit checkout for interacting with GitHub
Created attachment 434922 [details]
Patch
Created attachment 435003 [details]
Patch
Created attachment 435024 [details]
Patch
Created attachment 435035 [details]
Patch
Created attachment 435043 [details]
Patch
Comment on attachment 435043 [details]
Patch
r=me
Committed r280739 (240325@main): <https://commits.webkit.org/240325@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435043 [details]. |