Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1566037:
Tracker/Spreadsheet sync: tool to sync tracker issues (Closed)
DescriptionTracker/Spreadsheet sync: tool to sync tracker issues
This can be useful for working with issues in a spreadsheet and making
bulk updates to tracker issues.
See known issues in the file for important known issues.
Patch Set 1 #
Total comments: 47
Patch Set 2 : fixes for review, whitespace #Patch Set 3 : remove .py suffix from filename #
Messages
Total messages: 6 (0 generated)
http://codereview.chromium.org/1566037/diff/1/2 File src/scripts/tracker_spreadsheet_sync.py (right): http://codereview.chromium.org/1566037/diff/1/2#newcode28 src/scripts/tracker_spreadsheet_sync.py:28: # This script can be used to migrate Issue Tracker issues between Issue Tracker This probably deserves to be a docstring, printable with a --help option. http://codereview.chromium.org/1566037/diff/1/2#newcode84 src/scripts/tracker_spreadsheet_sync.py:84: def _PrintFeed(self, feed): FWIW, Python does some name munging for method names that start with a double underscore: >>> class Sample: ... def __mymethod(x): pass ... >>> dir(Sample) ['_Sample__mymethod', '__doc__', '__module__'] Google Python style suggests using a leading __ for private, but PEP8 suggests only using it to avoid name collisions in classes designed to be subclassed. http://codereview.chromium.org/1566037/diff/1/2#newcode86 src/scripts/tracker_spreadsheet_sync.py:86: for i, entry in enumerate(feed.entry): Cool, I hadn't actually seen enumerate() before :) http://codereview.chromium.org/1566037/diff/1/2#newcode102 src/scripts/tracker_spreadsheet_sync.py:102: def trackerLogin(self): Google Python style and PEP8 both agree on lowercase_underscores() for method names. They also agree that methods of a nontrivial length should have a helpful docstring :) http://codereview.chromium.org/1566037/diff/1/2#newcode114 src/scripts/tracker_spreadsheet_sync.py:114: self.it_client.ClientLogin(self.tracker_user, password, Clearly Google isn't following its own naming convention here :) http://codereview.chromium.org/1566037/diff/1/2#newcode138 src/scripts/tracker_spreadsheet_sync.py:138: for i, entry in enumerate(feed.entry): You're not actually using the variable i in the loop body, so: for entry in feed.entry: ...should suffice. http://codereview.chromium.org/1566037/diff/1/2#newcode155 src/scripts/tracker_spreadsheet_sync.py:155: if len(feed.entry) is 0: if not feed.entry: is more pythonic. http://codereview.chromium.org/1566037/diff/1/2#newcode161 src/scripts/tracker_spreadsheet_sync.py:161: issue_dict['labels'] = label_texts Why the intermediate label_texts variable? http://codereview.chromium.org/1566037/diff/1/2#newcode242 src/scripts/tracker_spreadsheet_sync.py:242: if ss_issue.has_key('status') and (ss_issue['status'] is not None): It's shorter to ask... if ss_issue.get('status') is not None: http://codereview.chromium.org/1566037/diff/1/2#newcode247 src/scripts/tracker_spreadsheet_sync.py:247: (ss_issue[key] is not None): If you're enumerating all keys in ss_issue, then ss_issue.has_key(key) will always be true, no? http://codereview.chromium.org/1566037/diff/1/2#newcode248 src/scripts/tracker_spreadsheet_sync.py:248: ret['labels'].append(key.title() + ss_issue[key]) This loop probably warrants... for (key, value) in ss_issue.items(): http://codereview.chromium.org/1566037/diff/1/2#newcode265 src/scripts/tracker_spreadsheet_sync.py:265: if ss_issue.has_key('owner') and (ss_issue['owner'] is not None): ss_issue.get('owner') http://codereview.chromium.org/1566037/diff/1/2#newcode271 src/scripts/tracker_spreadsheet_sync.py:271: for key in ss_issue.keys(): for (key, value) in ss_issue.items(): http://codereview.chromium.org/1566037/diff/1/2#newcode276 src/scripts/tracker_spreadsheet_sync.py:276: if ss_issue.has_key(key) and (ss_issue[key] is not None): get() http://codereview.chromium.org/1566037/diff/1/2#newcode280 src/scripts/tracker_spreadsheet_sync.py:280: if (t_label is None) and (ss_label is None): These parens aren't needed http://codereview.chromium.org/1566037/diff/1/2#newcode349 src/scripts/tracker_spreadsheet_sync.py:349: if len(commits) == 0: if not committs: Looks like you could return early from here too, or for extra python bonus points... >>> for i in []: ... pass ... else: ... print "none" ... none http://codereview.chromium.org/1566037/diff/1/2#newcode354 src/scripts/tracker_spreadsheet_sync.py:354: labels = None labels = dic.get('labels') http://codereview.chromium.org/1566037/diff/1/2#newcode393 src/scripts/tracker_spreadsheet_sync.py:393: def spreadsheetIssueForId(self, issues, the_id): I believe Python convention for avoiding global name clashes would suggest 'id_'. http://codereview.chromium.org/1566037/diff/1/2#newcode412 src/scripts/tracker_spreadsheet_sync.py:412: if key in self.it_keys: if key in self.it_keys and key in t_issue: http://codereview.chromium.org/1566037/diff/1/2#newcode450 src/scripts/tracker_spreadsheet_sync.py:450: print 'Error: must have at least one non-header row in spreadsheet' Should this be an exception? http://codereview.chromium.org/1566037/diff/1/2#newcode456 src/scripts/tracker_spreadsheet_sync.py:456: if self.debug: Might be cleaner to have a self.debug() method that checks if self.__verbose_level, or somesuch. http://codereview.chromium.org/1566037/diff/1/2#newcode458 src/scripts/tracker_spreadsheet_sync.py:458: if len(ss_commits) == 0: if not ss_commits:, or the for ... else: pattern
PTAL thanks http://codereview.chromium.org/1566037/diff/1/2 File src/scripts/tracker_spreadsheet_sync.py (right): http://codereview.chromium.org/1566037/diff/1/2#newcode28 src/scripts/tracker_spreadsheet_sync.py:28: # This script can be used to migrate Issue Tracker issues between Issue Tracker On 2010/04/12 18:01:20, rginda wrote: > This probably deserves to be a docstring, printable with a --help option. Done. http://codereview.chromium.org/1566037/diff/1/2#newcode84 src/scripts/tracker_spreadsheet_sync.py:84: def _PrintFeed(self, feed): On 2010/04/12 18:01:20, rginda wrote: > FWIW, Python does some name munging for method names that start with a double > underscore: > > >>> class Sample: > ... def __mymethod(x): pass > ... > >>> dir(Sample) > ['_Sample__mymethod', '__doc__', '__module__'] > > Google Python style suggests using a leading __ for private, but PEP8 suggests > only using it to avoid name collisions in classes designed to be subclassed. Done. http://codereview.chromium.org/1566037/diff/1/2#newcode86 src/scripts/tracker_spreadsheet_sync.py:86: for i, entry in enumerate(feed.entry): On 2010/04/12 18:01:20, rginda wrote: > Cool, I hadn't actually seen enumerate() before :) Me neither http://codereview.chromium.org/1566037/diff/1/2#newcode102 src/scripts/tracker_spreadsheet_sync.py:102: def trackerLogin(self): On 2010/04/12 18:01:20, rginda wrote: > Google Python style and PEP8 both agree on lowercase_underscores() for method > names. They also agree that methods of a nontrivial length should have a > helpful docstring :) Done. http://codereview.chromium.org/1566037/diff/1/2#newcode114 src/scripts/tracker_spreadsheet_sync.py:114: self.it_client.ClientLogin(self.tracker_user, password, On 2010/04/12 18:01:20, rginda wrote: > Clearly Google isn't following its own naming convention here :) yeah http://codereview.chromium.org/1566037/diff/1/2#newcode138 src/scripts/tracker_spreadsheet_sync.py:138: for i, entry in enumerate(feed.entry): On 2010/04/12 18:01:20, rginda wrote: > You're not actually using the variable i in the loop body, so: > > for entry in feed.entry: > > ...should suffice. Done. http://codereview.chromium.org/1566037/diff/1/2#newcode155 src/scripts/tracker_spreadsheet_sync.py:155: if len(feed.entry) is 0: On 2010/04/12 18:01:20, rginda wrote: > if not feed.entry: > > is more pythonic. Done. http://codereview.chromium.org/1566037/diff/1/2#newcode161 src/scripts/tracker_spreadsheet_sync.py:161: issue_dict['labels'] = label_texts On 2010/04/12 18:01:20, rginda wrote: > Why the intermediate label_texts variable? sample code did it that way. fixed. http://codereview.chromium.org/1566037/diff/1/2#newcode242 src/scripts/tracker_spreadsheet_sync.py:242: if ss_issue.has_key('status') and (ss_issue['status'] is not None): On 2010/04/12 18:01:20, rginda wrote: > It's shorter to ask... > > if ss_issue.get('status') is not None: Done. http://codereview.chromium.org/1566037/diff/1/2#newcode247 src/scripts/tracker_spreadsheet_sync.py:247: (ss_issue[key] is not None): On 2010/04/12 18:01:20, rginda wrote: > If you're enumerating all keys in ss_issue, then ss_issue.has_key(key) will > always be true, no? yes. fixed. http://codereview.chromium.org/1566037/diff/1/2#newcode248 src/scripts/tracker_spreadsheet_sync.py:248: ret['labels'].append(key.title() + ss_issue[key]) On 2010/04/12 18:01:20, rginda wrote: > This loop probably warrants... > > for (key, value) in ss_issue.items(): > Done. http://codereview.chromium.org/1566037/diff/1/2#newcode265 src/scripts/tracker_spreadsheet_sync.py:265: if ss_issue.has_key('owner') and (ss_issue['owner'] is not None): On 2010/04/12 18:01:20, rginda wrote: > ss_issue.get('owner') Done. http://codereview.chromium.org/1566037/diff/1/2#newcode271 src/scripts/tracker_spreadsheet_sync.py:271: for key in ss_issue.keys(): On 2010/04/12 18:01:20, rginda wrote: > for (key, value) in ss_issue.items(): Done. http://codereview.chromium.org/1566037/diff/1/2#newcode276 src/scripts/tracker_spreadsheet_sync.py:276: if ss_issue.has_key(key) and (ss_issue[key] is not None): On 2010/04/12 18:01:20, rginda wrote: > get() changed to simply 'value' which should be okay, right? http://codereview.chromium.org/1566037/diff/1/2#newcode280 src/scripts/tracker_spreadsheet_sync.py:280: if (t_label is None) and (ss_label is None): On 2010/04/12 18:01:20, rginda wrote: > These parens aren't needed Done. http://codereview.chromium.org/1566037/diff/1/2#newcode349 src/scripts/tracker_spreadsheet_sync.py:349: if len(commits) == 0: On 2010/04/12 18:01:20, rginda wrote: > if not committs: > > Looks like you could return early from here too, or for extra python bonus > points... > > >>> for i in []: > ... pass > ... else: > ... print "none" > ... > none is that really considered better? http://codereview.chromium.org/1566037/diff/1/2#newcode354 src/scripts/tracker_spreadsheet_sync.py:354: labels = None On 2010/04/12 18:01:20, rginda wrote: > labels = dic.get('labels') Done. http://codereview.chromium.org/1566037/diff/1/2#newcode393 src/scripts/tracker_spreadsheet_sync.py:393: def spreadsheetIssueForId(self, issues, the_id): On 2010/04/12 18:01:20, rginda wrote: > I believe Python convention for avoiding global name clashes would suggest > 'id_'. Done. http://codereview.chromium.org/1566037/diff/1/2#newcode412 src/scripts/tracker_spreadsheet_sync.py:412: if key in self.it_keys: On 2010/04/12 18:01:20, rginda wrote: > if key in self.it_keys and key in t_issue: > Done. http://codereview.chromium.org/1566037/diff/1/2#newcode450 src/scripts/tracker_spreadsheet_sync.py:450: print 'Error: must have at least one non-header row in spreadsheet' On 2010/04/12 18:01:20, rginda wrote: > Should this be an exception? Done. http://codereview.chromium.org/1566037/diff/1/2#newcode456 src/scripts/tracker_spreadsheet_sync.py:456: if self.debug: On 2010/04/12 18:01:20, rginda wrote: > Might be cleaner to have a self.debug() method that checks if > self.__verbose_level, or somesuch. broke it into a function, but left it a simple bool for now. if there are verbosity levels in the future it should be easier to integrate that. http://codereview.chromium.org/1566037/diff/1/2#newcode458 src/scripts/tracker_spreadsheet_sync.py:458: if len(ss_commits) == 0: On 2010/04/12 18:01:20, rginda wrote: > if not ss_commits:, or the for ... else: pattern Done.
I suggest remove the .py suffix. I believe convention is no suffix for main executables (and .sh/.py suffix for libraries).
PTAL, thanks
LGTM http://codereview.chromium.org/1566037/diff/1/2 File src/scripts/tracker_spreadsheet_sync.py (right): http://codereview.chromium.org/1566037/diff/1/2#newcode276 src/scripts/tracker_spreadsheet_sync.py:276: if ss_issue.has_key(key) and (ss_issue[key] is not None): On 2010/04/13 03:15:14, adlr wrote: > On 2010/04/12 18:01:20, rginda wrote: > > get() > > changed to simply 'value' which should be okay, right? yeah, sgtm http://codereview.chromium.org/1566037/diff/1/2#newcode349 src/scripts/tracker_spreadsheet_sync.py:349: if len(commits) == 0: On 2010/04/13 03:15:14, adlr wrote: > On 2010/04/12 18:01:20, rginda wrote: > > if not committs: > > > > Looks like you could return early from here too, or for extra python bonus > > points... > > > > >>> for i in []: > > ... pass > > ... else: > > ... print "none" > > ... > > none > > is that really considered better? It got added to the language because They thought so. I haven't tried it enough to know if I agree. I'm fine with the separate test, but you should at least switch to 'if not commits:' http://codereview.chromium.org/1566037/diff/1/2#newcode456 src/scripts/tracker_spreadsheet_sync.py:456: if self.debug: On 2010/04/13 03:15:14, adlr wrote: > On 2010/04/12 18:01:20, rginda wrote: > > Might be cleaner to have a self.debug() method that checks if > > self.__verbose_level, or somesuch. > > broke it into a function, but left it a simple bool for now. if there are > verbosity levels in the future it should be easier to integrate that. Sorry, I wasn't clear. I'm not concerned with adding verbosity levels, I was just suggesting swapping the: if self.debug: print msg pattern with: self.debug(msg) |
|||||||||||||||||||||||||||||||||||||||||||
