When using the webkitpy.common.checkout.scm module, it is often useful to determine the status of the current checkout. The SVN class provides a solution using regular expressions, but it is not very extensible and duplicates quite a bit of code. Since the status codes correspond to a well-defined bitmask-like syntax, we can do better than resorting to calls like svn_instance.run_status_and_extract_filenames(self.status_command(), self._status_regexp("?")) when something like svn_instance.unversioned_files() would be much more clear.
Created attachment 231854 [details] Patch
Comment on attachment 231854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231854&action=review Do we really need to support all of them? > Tools/Scripts/webkitpy/common/checkout/changelog.py:31 > +import codecs Why do we need to import codecs now? > Tools/Scripts/webkitpy/common/checkout/scm/git.py:-241 > - def added_files(self): > - return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("A")) > - > - def deleted_files(self): > - return self.run_status_and_extract_filenames(self.status_command(), self._status_regexp("D")) > - Doesn't this break git?
Comment on attachment 231854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231854&action=review >> Tools/Scripts/webkitpy/common/checkout/changelog.py:31 >> +import codecs > > Why do we need to import codecs now? This is an artifact. I will remove it. >> Tools/Scripts/webkitpy/common/checkout/scm/git.py:-241 >> - > > Doesn't this break git? This does not break git, because the git implementation of status_command() is not `git st`, but `git diff --name-status --no-renames HEAD`
Comment on attachment 231854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231854&action=review > Tools/Scripts/webkitpy/common/checkout/scm/status.py:6 > + self.file_statuses = [FileStatus(line, checkout_root) for line in text.split("\n") if line] We usually prefix private variables with _. > Tools/Scripts/webkitpy/common/checkout/scm/status.py:45 > +class FileStatus(object): FileStatus is an extremely generic name. Please give it a more descriptive name. > Tools/Scripts/webkitpy/common/checkout/scm/status.py:73 > + # Status convenience methods Can't we use __getattr__ to implement these instead? > Tools/Scripts/webkitpy/common/checkout/scm/status.py:175 > +class Code(object): Code is an extremely generic class name. Please give it a more descriptive name. > Tools/Scripts/webkitpy/common/checkout/scm/status.py:194 > +ItemReplaced = Code(1, "R", "Item has been replaced in your working " > + "copy. This means the file was scheduled " > + "for deletion, and then a new file with " > + "the same name was scheduled for addition " > + "in its place.") We don't align text line this with indentations. Please indent each line with exactly 4 spaces. Also, these descriptions are split into so many lines. We can probably fit this into much fewer lines.
Comment on attachment 231854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231854&action=review I will send a v2 with Ryosuke's feedback incorporated. > Tools/Scripts/webkitpy/common/checkout/scm/status.py:4 > +class CheckoutStatus(object): SVNStyleCheckoutStatus >> Tools/Scripts/webkitpy/common/checkout/scm/status.py:6 >> + self.file_statuses = [FileStatus(line, checkout_root) for line in text.split("\n") if line] > > We usually prefix private variables with _. Noted. I will add the _ prefix. >> Tools/Scripts/webkitpy/common/checkout/scm/status.py:45 >> +class FileStatus(object): > > FileStatus is an extremely generic name. Please give it a more descriptive name. SVNStyleStatusCodeString >> Tools/Scripts/webkitpy/common/checkout/scm/status.py:175 >> +class Code(object): > > Code is an extremely generic class name. Please give it a more descriptive name. SVNStyleStatusCode >> Tools/Scripts/webkitpy/common/checkout/scm/status.py:194 >> + "in its place.") > > We don't align text line this with indentations. Please indent each line with exactly 4 spaces. > Also, these descriptions are split into so many lines. We can probably fit this into much fewer lines. Will do.
Comment on attachment 231854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231854&action=review >>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:-241 >>> - >> >> Doesn't this break git? > > This does not break git, because the git implementation of status_command() is not `git st`, but `git diff --name-status --no-renames HEAD` I will move the previous SCM implementation into git.py, so as to avoid any changes when using the scm module with Git clients. >> Tools/Scripts/webkitpy/common/checkout/scm/status.py:4 >> +class CheckoutStatus(object): > > SVNStyleCheckoutStatus EDIT: SVNCheckoutStatus >>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:45 >>> +class FileStatus(object): >> >> FileStatus is an extremely generic name. Please give it a more descriptive name. > > SVNStyleStatusCodeString EDIT: SVNStatusLineItem >> Tools/Scripts/webkitpy/common/checkout/scm/status.py:73 >> + # Status convenience methods > > Can't we use __getattr__ to implement these instead? By this, I assume you mean that we should use the Dispatch pattern with __getattr__? We already have that, in essence, with the matches_code method. There is an alternative way to do this with setattr by defining a method like: def _define_matching_function(self, svn_status_code): def matching_function(): return self.matches_status_code(svn_status_code) return matching_function and then looping over all of the property constants and setting their snake_case representation to the function returned by _define_matching_function(). I find this hard to follow and unnecessarily complicated for a set of constants that will not be changing anytime soon. Are you ok with keeping these properties explicitly defined? >>> Tools/Scripts/webkitpy/common/checkout/scm/status.py:175 >>> +class Code(object): >> >> Code is an extremely generic class name. Please give it a more descriptive name. > > SVNStyleStatusCode EDIT: SVNStatusCode
Created attachment 231979 [details] Patch
Comment on attachment 231979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231979&action=review (In reply to comment #6) > > By this, I assume you mean that we should use the Dispatch pattern with __getattr__? We already have that, in essence, with the matches_code method. There is an alternative way to do this with setattr by defining a method like: > > def _define_matching_function(self, svn_status_code): > def matching_function(): > return self.matches_status_code(svn_status_code) > return matching_function > > and then looping over all of the property constants and setting their snake_case representation to the function returned by _define_matching_function(). I find this hard to follow and unnecessarily complicated for a set of constants that will not be changing anytime soon. > > Are you ok with keeping these properties explicitly defined? Why do they need to be actual properties? Can't they just be method calls in call sites? If we did that, then we can simply iterate through the list of status codes and check whether it matches or not. Alternatively, why can't we just export status objects and let call sites manually call matches_status_code? I don't like the fact we're listing every status code name almost thrice in SVNCheckoutStatus, SVNStatusLineItem, and for status code objects themselves. > Tools/Scripts/webkitpy/common/checkout/scm/svn_status.py:78 > + def _define_matching_function(self, svn_status_code): > + def matching_function(): > + return self.matches_status_code(svn_status_code) > + return matching_function Where is this used?
Comment on attachment 231979 [details] Patch This patch is too old to apply now. Can you revise against current sources?
@Brent & @Ryosuke - do we need this considering we have now moved to Github? Thanks!
Yeah, this is won't fix.