Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873016:
First cut at stable script (Closed)
|
Created:
10 years, 6 months ago by sosa Modified:
9 years, 6 months ago CC:
chromium-os-reviews_chromium.org, anush, sosa, Mandeep Singh Baines Base URL:
ssh://git@chromiumos-git//crosutils.git Visibility:
Public. |
DescriptionFirst cut at stable script
TEST=Tested by using crash-reporter and crash_ids set to "12345"
Patch Set 1 #Patch Set 2 : Clean up and OO #
Total comments: 8
Patch Set 3 : Fixes for msb #Patch Set 4 : Unit tests and clean up #Patch Set 5 : unit tests #Patch Set 6 : Fix 80 chars #
Total comments: 104
Patch Set 7 : petkov refactor #Patch Set 8 : Forgot symlink. #
Total comments: 72
Patch Set 9 : Fixes for petkov #
Total comments: 10
Patch Set 10 : Petkov #
Total comments: 14
Patch Set 11 : msb fixes #Patch Set 12 : Nits #
Messages
Total messages: 16 (0 generated)
First commit ready for this script. Usage: ./cros_mark_as_stable -p <package_list> -i <optional commit hashes> Output - Create a branch called stabilizing_branch in the chromiumos-overlay that stabilizes the next 9999 ebuild, rev's it, and places new EGIT_COMMIT id's into the new ebuild based on "commit_ids" Push not currently implemented (well code is there but I'm not going to put a --bypass-hooks in until it's ready). Left to do: Either break the push part into a separate script or the same script with different paramaters so that it just uses the stabilizing_branch and pushes the changes after tests have passed.
Ping!
looks like a good start http://codereview.chromium.org/2873016/diff/2001/3001 File cros_mark_as_stable (right): http://codereview.chromium.org/2873016/diff/2001/3001#newcode84 cros_mark_as_stable:84: #self.DeleteBranch() Uncomment http://codereview.chromium.org/2873016/diff/2001/3001#newcode127 cros_mark_as_stable:127: ebuild_no_version = ebuild_no_rev.rpartition('-')[0] I don't think this will work for the case of no -r. >>> x = "fsadfds-r4" >>> x.rpartition('-') ('fsadfds', '-', 'r4') >>> x = "fsadfds" >>> x.rpartition('-') ('', '', 'fsadfds') http://codereview.chromium.org/2873016/diff/2001/3001#newcode209 cros_mark_as_stable:209: print 'Unable to create stabilizing branch' Return non-zero in this case. http://codereview.chromium.org/2873016/diff/2001/3001#newcode216 cros_mark_as_stable:216: if commit_id_list: What happens if there is no commit_id. I don't think you can do anything at that point. Maybe print and error and exit.
PTAL http://codereview.chromium.org/2873016/diff/2001/3001 File cros_mark_as_stable (right): http://codereview.chromium.org/2873016/diff/2001/3001#newcode84 cros_mark_as_stable:84: #self.DeleteBranch() Removed. On 2010/06/30 02:38:19, Mandeep Singh Baines wrote: > Uncomment http://codereview.chromium.org/2873016/diff/2001/3001#newcode127 cros_mark_as_stable:127: ebuild_no_version = ebuild_no_rev.rpartition('-')[0] You're right. I hadn't tested without -r. Cleaned up and fixed and tested with and without r. On 2010/06/30 02:38:19, Mandeep Singh Baines wrote: > I don't think this will work for the case of no -r. > > >>> x = "fsadfds-r4" > >>> x.rpartition('-') > ('fsadfds', '-', 'r4') > >>> x = "fsadfds" > >>> x.rpartition('-') > ('', '', 'fsadfds') > http://codereview.chromium.org/2873016/diff/2001/3001#newcode209 cros_mark_as_stable:209: print 'Unable to create stabilizing branch' On 2010/06/30 02:38:19, Mandeep Singh Baines wrote: > Return non-zero in this case. Done. http://codereview.chromium.org/2873016/diff/2001/3001#newcode216 cros_mark_as_stable:216: if commit_id_list: Before the buildbot uses this, I want developers to use this script to be able to uprev their own packages easily. This is why I'm making the commit_id's optional in the script. On 2010/06/30 02:38:19, Mandeep Singh Baines wrote: > What happens if there is no commit_id. I don't think you can do anything at that > point. Maybe print and error and exit.
Ping. Also have added unit tests if you want to take a look. Adding +petkov to take a harder look at my unit tests.
I made an initial pass and took the liberty to review the whole CL, mostly for style. One general comment: review/test the code from robustness standpoint (e.g., what happens if any of the git commands fail). Feel free to ignore any of the comments, especially if this needs to get in to move the build system forward. http://codereview.chromium.org/2873016/diff/16001/17002 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/16001/17002#newcode7 cros_mark_as_stable.py:7: """ This module uprevs a given package's ebuild to the next revision """ End comment with a period. http://codereview.chromium.org/2873016/diff/16001/17002#newcode36 cros_mark_as_stable.py:36: # TODO: Remove hard-coding of overlays directory once we find a better way. Per style, add you username to the TODO. http://codereview.chromium.org/2873016/diff/16001/17002#newcode37 cros_mark_as_stable.py:37: CHROMIUMOS_OVERLAYS_DIRECTORY = \ Is this meant to be a public constant? If not, prefix with _. http://codereview.chromium.org/2873016/diff/16001/17002#newcode40 cros_mark_as_stable.py:40: # ======================= Global Helper Functions ======================== Are these functions meant to be public? Per style, if internal only, they should be prefixed with _. http://codereview.chromium.org/2873016/diff/16001/17002#newcode43 cros_mark_as_stable.py:43: def VPrint(message): I'd avoid V as an abbreviation. Maybe just Print? http://codereview.chromium.org/2873016/diff/16001/17002#newcode44 cros_mark_as_stable.py:44: """ Verbose print function. """ I don't think you're supposed to have spaces after and before """ (per style examples). Throughout. http://codereview.chromium.org/2873016/diff/16001/17002#newcode49 cros_mark_as_stable.py:49: def ArgumentsAreSane(package_list, commit_id_list): I think there's an (unwritten?) style rule to start function names with verbs, i.e., something like CheckSaneArguments. http://codereview.chromium.org/2873016/diff/16001/17002#newcode52 cros_mark_as_stable.py:52: print 'Please specify at least one package' You should really invoke a Die function here with a message. and then the function doesn't need to return status. Also, consider adding color :-) See scripts/generate_test_report for example... Maybe a separate CL. http://codereview.chromium.org/2873016/diff/16001/17002#newcode54 cros_mark_as_stable.py:54: elif not gflags.FLAGS.board: elif here, no elif below. I'd remove the elif here. http://codereview.chromium.org/2873016/diff/16001/17002#newcode63 cros_mark_as_stable.py:63: def PrintUsageAndDie(): Again, this could be (or go through) a generic Die function. http://codereview.chromium.org/2873016/diff/16001/17002#newcode70 cros_mark_as_stable.py:70: """ Runs a shell command and returns stdout back to caller. """ Exist status is ignored? Or generates an exception? It might make sense to document. http://codereview.chromium.org/2873016/diff/16001/17002#newcode81 cros_mark_as_stable.py:81: def __init__(self, branch_name): Per style, one blank line between class and first method. http://codereview.chromium.org/2873016/diff/16001/17002#newcode84 cros_mark_as_stable.py:84: Delete an existing branch if found and creates a new one. s/Delete/Deletes/ Or you could just say the the function """Creates a new git branch or replaces an existing one.""" http://codereview.chromium.org/2873016/diff/16001/17002#newcode86 cros_mark_as_stable.py:86: self.branch_name = branch_name self._branch_name, if private/internal. http://codereview.chromium.org/2873016/diff/16001/17002#newcode87 cros_mark_as_stable.py:87: if self.BranchExists(): This seems like a bit too much work for a constructor, if it was C++ -- you would move most of this into an Init function. Not sure if there's any rule about this in Python... http://codereview.chromium.org/2873016/diff/16001/17002#newcode92 cros_mark_as_stable.py:92: """ Cleans up by checking out to master """ Period at the end. http://codereview.chromium.org/2873016/diff/16001/17002#newcode101 cros_mark_as_stable.py:101: RunCommand(git_cmd) It seems you want a generic RunGit routine too. http://codereview.chromium.org/2873016/diff/16001/17002#newcode103 cros_mark_as_stable.py:103: def BranchExists(self): Given that you're inside the GitBranch class, maybe rename to "Exists"? http://codereview.chromium.org/2873016/diff/16001/17002#newcode107 cros_mark_as_stable.py:107: return self.branch_name in branches Does it make sense to make this a bit more robust now so that you really match a whole branch name, not just a substring? I.e., split on spaces, etc. http://codereview.chromium.org/2873016/diff/16001/17002#newcode109 cros_mark_as_stable.py:109: def DeleteBranch(self): Given that you're inside the GitBranch class, maybe rename to "Delete"? http://codereview.chromium.org/2873016/diff/16001/17002#newcode110 cros_mark_as_stable.py:110: """ Delete the branch and return the user to the master branch """ s/Delete/Deletes/ and /return/returns/ and period at the end. http://codereview.chromium.org/2873016/diff/16001/17002#newcode118 cros_mark_as_stable.py:118: def __init__(self, package, commit_id=None): Add a blank line before __init__ http://codereview.chromium.org/2873016/diff/16001/17002#newcode125 cros_mark_as_stable.py:125: self.ebuild_path = self._FindEBuildPath(package) Again, consider moving some of this into an Init method instead. http://codereview.chromium.org/2873016/diff/16001/17002#newcode125 cros_mark_as_stable.py:125: self.ebuild_path = self._FindEBuildPath(package) What happens downstream if you don't find the package? http://codereview.chromium.org/2873016/diff/16001/17002#newcode129 cros_mark_as_stable.py:129: self.commit_id = commit_id Is this commit_id used? http://codereview.chromium.org/2873016/diff/16001/17002#newcode131 cros_mark_as_stable.py:131: @classmethod Why don't you use @staticmethod instead? And then there's no need for 'cls' below. http://codereview.chromium.org/2873016/diff/16001/17002#newcode169 cros_mark_as_stable.py:169: def __init__(self, ebuild): Blank line. http://codereview.chromium.org/2873016/diff/16001/17002#newcode170 cros_mark_as_stable.py:170: self.ebuild = ebuild _ebuild. http://codereview.chromium.org/2873016/diff/16001/17002#newcode173 cros_mark_as_stable.py:173: """ Revs an ebuild given the git commit id Period. http://codereview.chromium.org/2873016/diff/16001/17002#newcode179 cros_mark_as_stable.py:179: # TODO: Change to a check? TODO(sosa) http://codereview.chromium.org/2873016/diff/16001/17002#newcode188 cros_mark_as_stable.py:188: up_rev_command = 'cp %s-9999.ebuild %s' % \ You should use shutil.copyfile instead. http://codereview.chromium.org/2873016/diff/16001/17002#newcode193 cros_mark_as_stable.py:193: for line in fileinput.input('%s' % new_ebuild_path, inplace=1): Isn't new_ebuild_path a string already? Why do you need the conversion? http://codereview.chromium.org/2873016/diff/16001/17002#newcode194 cros_mark_as_stable.py:194: # For issues related to scoping, this has to be done here for fileinput. This seems ugly... There must be a better way :-) http://codereview.chromium.org/2873016/diff/16001/17002#newcode206 cros_mark_as_stable.py:206: file_to_write.write('EGIT_COMMIT=\"%s\"' % commit_id) No need to \", just " should work I think. http://codereview.chromium.org/2873016/diff/16001/17002#newcode219 cros_mark_as_stable.py:219: def CommitChange(self, message): you need more documentation for your public methods, I think. See style guide. http://codereview.chromium.org/2873016/diff/16001/17002#newcode220 cros_mark_as_stable.py:220: """ Commits the current changes in the current branch locally """ Period. http://codereview.chromium.org/2873016/diff/16001/17002#newcode223 cros_mark_as_stable.py:223: git_commit_cmd = 'git commit -am \"%s\"' % message No need for \", just " http://codereview.chromium.org/2873016/diff/16001/17002#newcode226 cros_mark_as_stable.py:226: Remove one blank line. http://codereview.chromium.org/2873016/diff/16001/17002#newcode227 cros_mark_as_stable.py:227: # TODO: This doesn't work yet. Want to directly push without a prompt. TODO(sosa) http://codereview.chromium.org/2873016/diff/16001/17002#newcode267 cros_mark_as_stable.py:267: print 'Push currently not implemented' TODO? http://codereview.chromium.org/2873016/diff/16001/17002#newcode274 cros_mark_as_stable.py:274: del work_branch Isn't the destructor run at exit always? Not sure... http://codereview.chromium.org/2873016/diff/16001/17003 File cros_mark_as_stable_unittest.py (right): http://codereview.chromium.org/2873016/diff/16001/17003#newcode16 cros_mark_as_stable_unittest.py:16: sys.path.append(os.path.dirname(os.path.dirname(__file__))) Aren't you going two levels up here by calling dirname twice? http://codereview.chromium.org/2873016/diff/16001/17003#newcode21 cros_mark_as_stable_unittest.py:21: def setUp(self): Add a blank line. http://codereview.chromium.org/2873016/diff/16001/17003#newcode21 cros_mark_as_stable_unittest.py:21: def setUp(self): Does setUp run once per each test in this class or just once? If once per each test, then it might make sense to move here some of the Mox creation and GitBranch creation common code from the individual test cases. http://codereview.chromium.org/2873016/diff/16001/17003#newcode24 cros_mark_as_stable_unittest.py:24: def testInit(self): Given how easy it is to mock methods, for a real unit test you should test _Checkout in a separate unit test. Then here, you should mock out _Checkout, BranchExists and DeleteBranch (because you already have unit tests for them). http://codereview.chromium.org/2873016/diff/16001/17003#newcode36 cros_mark_as_stable_unittest.py:36: # Test init with previous branch existing. Split into a separate test? http://codereview.chromium.org/2873016/diff/16001/17003#newcode71 cros_mark_as_stable_unittest.py:71: self.branch) Do you want to return a list of branches instead? http://codereview.chromium.org/2873016/diff/16001/17003#newcode79 cros_mark_as_stable_unittest.py:79: def setUp(self): Add a blank line. http://codereview.chromium.org/2873016/diff/16001/17003#newcode124 cros_mark_as_stable_unittest.py:124: # Test with ebuild without revision number. Separate test. http://codereview.chromium.org/2873016/diff/16001/17003#newcode175 cros_mark_as_stable_unittest.py:175: cros_mark_as_stable.RunCommand(mox.StrContains( Can you match an exact string here, rather than a substring? Here and other places...
Thanks Darin. PTAL http://codereview.chromium.org/2873016/diff/16001/17002 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/16001/17002#newcode7 cros_mark_as_stable.py:7: """ This module uprevs a given package's ebuild to the next revision """ On 2010/07/01 23:59:40, petkov wrote: > End comment with a period. Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode36 cros_mark_as_stable.py:36: # TODO: Remove hard-coding of overlays directory once we find a better way. On 2010/07/01 23:59:40, petkov wrote: > Per style, add you username to the TODO. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode37 cros_mark_as_stable.py:37: CHROMIUMOS_OVERLAYS_DIRECTORY = \ On 2010/07/01 23:59:40, petkov wrote: > Is this meant to be a public constant? If not, prefix with _. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode40 cros_mark_as_stable.py:40: # ======================= Global Helper Functions ======================== On 2010/07/01 23:59:40, petkov wrote: > Are these functions meant to be public? Per style, if internal only, they should > be prefixed with _. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode43 cros_mark_as_stable.py:43: def VPrint(message): On 2010/07/01 23:59:40, petkov wrote: > I'd avoid V as an abbreviation. Maybe just Print? > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode44 cros_mark_as_stable.py:44: """ Verbose print function. """ On 2010/07/01 23:59:40, petkov wrote: > I don't think you're supposed to have spaces after and before """ (per style > examples). Throughout. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode49 cros_mark_as_stable.py:49: def ArgumentsAreSane(package_list, commit_id_list): On 2010/07/01 23:59:40, petkov wrote: > I think there's an (unwritten?) style rule to start function names with verbs, > i.e., something like CheckSaneArguments. Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode52 cros_mark_as_stable.py:52: print 'Please specify at least one package' On 2010/07/01 23:59:40, petkov wrote: > You should really invoke a Die function here with a message. and then the > function doesn't need to return status. > > Also, consider adding color :-) See scripts/generate_test_report for example... > Maybe a separate CL. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode54 cros_mark_as_stable.py:54: elif not gflags.FLAGS.board: On 2010/07/01 23:59:40, petkov wrote: > elif here, no elif below. I'd remove the elif here. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode63 cros_mark_as_stable.py:63: def PrintUsageAndDie(): On 2010/07/01 23:59:40, petkov wrote: > Again, this could be (or go through) a generic Die function. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode70 cros_mark_as_stable.py:70: """ Runs a shell command and returns stdout back to caller. """ On 2010/07/01 23:59:40, petkov wrote: > Exist status is ignored? Or generates an exception? It might make sense to > document. Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode81 cros_mark_as_stable.py:81: def __init__(self, branch_name): On 2010/07/01 23:59:40, petkov wrote: > Per style, one blank line between class and first method. Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode84 cros_mark_as_stable.py:84: Delete an existing branch if found and creates a new one. On 2010/07/01 23:59:40, petkov wrote: > s/Delete/Deletes/ > > Or you could just say the the function > """Creates a new git branch or replaces an existing one.""" > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode86 cros_mark_as_stable.py:86: self.branch_name = branch_name it's not. On 2010/07/01 23:59:40, petkov wrote: > self._branch_name, if private/internal. > http://codereview.chromium.org/2873016/diff/16001/17002#newcode87 cros_mark_as_stable.py:87: if self.BranchExists(): On 2010/07/01 23:59:40, petkov wrote: > This seems like a bit too much work for a constructor, if it was C++ -- you > would move most of this into an Init function. Not sure if there's any rule > about this in Python... > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode92 cros_mark_as_stable.py:92: """ Cleans up by checking out to master """ On 2010/07/01 23:59:40, petkov wrote: > Period at the end. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode101 cros_mark_as_stable.py:101: RunCommand(git_cmd) I thought about this too but it seemed silly to re-use sep for git and for equery. On 2010/07/01 23:59:40, petkov wrote: > It seems you want a generic RunGit routine too. > http://codereview.chromium.org/2873016/diff/16001/17002#newcode103 cros_mark_as_stable.py:103: def BranchExists(self): On 2010/07/01 23:59:40, petkov wrote: > Given that you're inside the GitBranch class, maybe rename to "Exists"? > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode107 cros_mark_as_stable.py:107: return self.branch_name in branches Great point! On 2010/07/01 23:59:40, petkov wrote: > Does it make sense to make this a bit more robust now so that you really match a > whole branch name, not just a substring? I.e., split on spaces, etc. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode109 cros_mark_as_stable.py:109: def DeleteBranch(self): On 2010/07/01 23:59:40, petkov wrote: > Given that you're inside the GitBranch class, maybe rename to "Delete"? > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode110 cros_mark_as_stable.py:110: """ Delete the branch and return the user to the master branch """ On 2010/07/01 23:59:40, petkov wrote: > s/Delete/Deletes/ and /return/returns/ and period at the end. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode118 cros_mark_as_stable.py:118: def __init__(self, package, commit_id=None): On 2010/07/01 23:59:40, petkov wrote: > Add a blank line before __init__ > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode125 cros_mark_as_stable.py:125: self.ebuild_path = self._FindEBuildPath(package) Throws an exception and moves on to the next package. On 2010/07/01 23:59:40, petkov wrote: > What happens downstream if you don't find the package? > http://codereview.chromium.org/2873016/diff/16001/17002#newcode125 cros_mark_as_stable.py:125: self.ebuild_path = self._FindEBuildPath(package) This pretty much is just a struct with initialization code so I don't think it's worth Init'fying On 2010/07/01 23:59:40, petkov wrote: > Again, consider moving some of this into an Init method instead. > http://codereview.chromium.org/2873016/diff/16001/17002#newcode129 cros_mark_as_stable.py:129: self.commit_id = commit_id Yes. Checkout out EbuildStableMarker::UpdateEBuild On 2010/07/01 23:59:40, petkov wrote: > Is this commit_id used? > http://codereview.chromium.org/2873016/diff/16001/17002#newcode131 cros_mark_as_stable.py:131: @classmethod I tried. Apparently "static methods" require @classmethod and cls. My previous googling of the issue gave me the wrong answer. On 2010/07/01 23:59:40, petkov wrote: > Why don't you use @staticmethod instead? And then there's no need for 'cls' > below. > > http://codereview.chromium.org/2873016/diff/16001/17002#newcode169 cros_mark_as_stable.py:169: def __init__(self, ebuild): On 2010/07/01 23:59:40, petkov wrote: > Blank line. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode170 cros_mark_as_stable.py:170: self.ebuild = ebuild On 2010/07/01 23:59:40, petkov wrote: > _ebuild. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode173 cros_mark_as_stable.py:173: """ Revs an ebuild given the git commit id On 2010/07/01 23:59:40, petkov wrote: > Period. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode179 cros_mark_as_stable.py:179: # TODO: Change to a check? On 2010/07/01 23:59:40, petkov wrote: > TODO(sosa) > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode193 cros_mark_as_stable.py:193: for line in fileinput.input('%s' % new_ebuild_path, inplace=1): On 2010/07/01 23:59:40, petkov wrote: > Isn't new_ebuild_path a string already? Why do you need the conversion? > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode194 cros_mark_as_stable.py:194: # For issues related to scoping, this has to be done here for fileinput. I'm not sure. I had a much cleaner way before but it seems like fileinput.input overrwrites sys.out especially when you use inplace=1. So you can't use the sys.out earlier to do the correct in place change. Another way is to create a temp file and re-write over this old file, but I think that might be even less clean. On 2010/07/01 23:59:40, petkov wrote: > This seems ugly... There must be a better way :-) > http://codereview.chromium.org/2873016/diff/16001/17002#newcode206 cros_mark_as_stable.py:206: file_to_write.write('EGIT_COMMIT=\"%s\"' % commit_id) On 2010/07/01 23:59:40, petkov wrote: > No need to \", just " should work I think. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode219 cros_mark_as_stable.py:219: def CommitChange(self, message): On 2010/07/01 23:59:40, petkov wrote: > you need more documentation for your public methods, I think. See style guide. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode220 cros_mark_as_stable.py:220: """ Commits the current changes in the current branch locally """ On 2010/07/01 23:59:40, petkov wrote: > Period. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode223 cros_mark_as_stable.py:223: git_commit_cmd = 'git commit -am \"%s\"' % message On 2010/07/01 23:59:40, petkov wrote: > No need for \", just " Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode226 cros_mark_as_stable.py:226: On 2010/07/01 23:59:40, petkov wrote: > Remove one blank line. > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode227 cros_mark_as_stable.py:227: # TODO: This doesn't work yet. Want to directly push without a prompt. On 2010/07/01 23:59:40, petkov wrote: > TODO(sosa) > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode267 cros_mark_as_stable.py:267: print 'Push currently not implemented' On 2010/07/01 23:59:40, petkov wrote: > TODO? > Done. http://codereview.chromium.org/2873016/diff/16001/17002#newcode274 cros_mark_as_stable.py:274: del work_branch Making it explicit ensures that it is not garbage collected early. On 2010/07/01 23:59:40, petkov wrote: > Isn't the destructor run at exit always? Not sure... > http://codereview.chromium.org/2873016/diff/16001/17003 File cros_mark_as_stable_unittest.py (right): http://codereview.chromium.org/2873016/diff/16001/17003#newcode16 cros_mark_as_stable_unittest.py:16: sys.path.append(os.path.dirname(os.path.dirname(__file__))) Yeah apparently it's recursive so I didn't catch it. Fixed. On 2010/07/01 23:59:40, petkov wrote: > Aren't you going two levels up here by calling dirname twice? > http://codereview.chromium.org/2873016/diff/16001/17003#newcode21 cros_mark_as_stable_unittest.py:21: def setUp(self): On 2010/07/01 23:59:40, petkov wrote: > Add a blank line. > Done. http://codereview.chromium.org/2873016/diff/16001/17003#newcode21 cros_mark_as_stable_unittest.py:21: def setUp(self): Refactored to use mox.TestBase which is a wrapper around unit test case that set up a mox. On 2010/07/01 23:59:40, petkov wrote: > Does setUp run once per each test in this class or just once? If once per each > test, then it might make sense to move here some of the Mox creation and > GitBranch creation common code from the individual test cases. > http://codereview.chromium.org/2873016/diff/16001/17003#newcode24 cros_mark_as_stable_unittest.py:24: def testInit(self): On 2010/07/01 23:59:40, petkov wrote: > Given how easy it is to mock methods, for a real unit test you should test > _Checkout in a separate unit test. > > Then here, you should mock out _Checkout, BranchExists and DeleteBranch (because > you already have unit tests for them). > Done. http://codereview.chromium.org/2873016/diff/16001/17003#newcode36 cros_mark_as_stable_unittest.py:36: # Test init with previous branch existing. On 2010/07/01 23:59:40, petkov wrote: > Split into a separate test? > Done. http://codereview.chromium.org/2873016/diff/16001/17003#newcode71 cros_mark_as_stable_unittest.py:71: self.branch) On 2010/07/01 23:59:40, petkov wrote: > Do you want to return a list of branches instead? > Done. http://codereview.chromium.org/2873016/diff/16001/17003#newcode79 cros_mark_as_stable_unittest.py:79: def setUp(self): On 2010/07/01 23:59:40, petkov wrote: > Add a blank line. > Done. http://codereview.chromium.org/2873016/diff/16001/17003#newcode124 cros_mark_as_stable_unittest.py:124: # Test with ebuild without revision number. On 2010/07/01 23:59:40, petkov wrote: > Separate test. > Done. http://codereview.chromium.org/2873016/diff/16001/17003#newcode175 cros_mark_as_stable_unittest.py:175: cros_mark_as_stable.RunCommand(mox.StrContains( On 2010/07/01 23:59:40, petkov wrote: > Can you match an exact string here, rather than a substring? Here and other > places... > Done.
Second round -- again, mostly style suggestions. http://codereview.chromium.org/2873016/diff/16001/17002 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/16001/17002#newcode70 cros_mark_as_stable.py:70: """ Runs a shell command and returns stdout back to caller. """ On 2010/07/02 23:26:08, sosa wrote: > On 2010/07/01 23:59:40, petkov wrote: > > Exist status is ignored? Or generates an exception? It might make sense to > > document. > > Done. I don't see this documented? http://codereview.chromium.org/2873016/diff/16001/17002#newcode101 cros_mark_as_stable.py:101: RunCommand(git_cmd) On 2010/07/02 23:26:09, sosa wrote: > I thought about this too but it seemed silly to re-use sep for git and for > equery. I'm not sure what you mean. You could still call RunCommand from RunGit. > > On 2010/07/01 23:59:40, petkov wrote: > > It seems you want a generic RunGit routine too. > > > > http://codereview.chromium.org/2873016/diff/16001/17002#newcode125 cros_mark_as_stable.py:125: self.ebuild_path = self._FindEBuildPath(package) On 2010/07/02 23:26:09, sosa wrote: > Throws an exception and moves on to the next package. This needs documentation, at least. Who throws the exception? How do you continue with the next package given that you're catching the exceptions _outside_ the package loop. > > On 2010/07/01 23:59:40, petkov wrote: > > What happens downstream if you don't find the package? > > > > http://codereview.chromium.org/2873016/diff/16001/17002#newcode129 cros_mark_as_stable.py:129: self.commit_id = commit_id On 2010/07/02 23:26:09, sosa wrote: > Yes. Checkout out EbuildStableMarker::UpdateEBuild I did (again). It seems to me that UpdateEBuild takes commit_id as an argument. So, EBuild.commit_id is unused. > > On 2010/07/01 23:59:40, petkov wrote: > > Is this commit_id used? > > > > http://codereview.chromium.org/2873016/diff/16001/17002#newcode194 cros_mark_as_stable.py:194: # For issues related to scoping, this has to be done here for fileinput. Does it work if you assign fileinput.input to a temp first, then you read sys.stdout, and then you do 'for line in temp'? On 2010/07/02 23:26:09, sosa wrote: > I'm not sure. I had a much cleaner way before but it seems like fileinput.input > overrwrites sys.out especially when you use inplace=1. So you can't use the > sys.out earlier to do the correct in place change. > > Another way is to create a temp file and re-write over this old file, but I > think that might be even less clean. > > On 2010/07/01 23:59:40, petkov wrote: > > This seems ugly... There must be a better way :-) > > > > http://codereview.chromium.org/2873016/diff/24001/9003#newcode19 cros_mark_as_stable.py:19: 'Board for which the package belongs', short_name='b') This seems unexpected. Add some comment why you're including this and maybe a TODO to refactor into a separate common library. http://codereview.chromium.org/2873016/diff/24001/9003#newcode23 cros_mark_as_stable.py:23: gflags.DEFINE_string('message', Period. http://codereview.chromium.org/2873016/diff/24001/9003#newcode25 cros_mark_as_stable.py:25: 'Commit message for change to ebuild.') ... of space-separated list... What if it's not supplied? Also, the number of commit id's must match the number of packages. All seems worth documenting. http://codereview.chromium.org/2873016/diff/24001/9003#newcode28 cros_mark_as_stable.py:28: short_name='p') %s's here appear in the usage, right? A bit ugly. Maybe put the default message format string in a separate variable and for the usage use <package> and <commit_id> in place of %s. http://codereview.chromium.org/2873016/diff/24001/9003#newcode40 cros_mark_as_stable.py:40: # ======================= Global Helper Functions ======================== 80 chars. http://codereview.chromium.org/2873016/diff/24001/9003#newcode56 cros_mark_as_stable.py:56: return False Die? http://codereview.chromium.org/2873016/diff/24001/9003#newcode59 cros_mark_as_stable.py:59: return False Die? http://codereview.chromium.org/2873016/diff/24001/9003#newcode62 cros_mark_as_stable.py:62: Die? http://codereview.chromium.org/2873016/diff/24001/9003#newcode69 cros_mark_as_stable.py:69: def RunCommand(command): It might sound like I'm changing my mind here but it might seem strange to print the whole usage in red. http://codereview.chromium.org/2873016/diff/24001/9003#newcode83 cros_mark_as_stable.py:83: Remove space at the end. http://codereview.chromium.org/2873016/diff/24001/9003#newcode85 cros_mark_as_stable.py:85: """ You probably need to document this as a public method. http://codereview.chromium.org/2873016/diff/24001/9003#newcode87 cros_mark_as_stable.py:87: if self.BranchExists(): _initialized is unused. Remove. http://codereview.chromium.org/2873016/diff/24001/9003#newcode92 cros_mark_as_stable.py:92: """ Cleans up by checking out to master """ Can you call CleanUp here instead? http://codereview.chromium.org/2873016/diff/24001/9003#newcode95 cros_mark_as_stable.py:95: def _Checkout(self, target, create=True): Do you think Init is the right name given the functionality? Maybe it is, just double checking... http://codereview.chromium.org/2873016/diff/24001/9003#newcode100 cros_mark_as_stable.py:100: git_cmd = 'git checkout %s' % target _initialized is unused. http://codereview.chromium.org/2873016/diff/24001/9003#newcode102 cros_mark_as_stable.py:102: Missing doc string for a public method. http://codereview.chromium.org/2873016/diff/24001/9003#newcode107 cros_mark_as_stable.py:107: return self.branch_name in branches Period. http://codereview.chromium.org/2873016/diff/24001/9003#newcode115 cros_mark_as_stable.py:115: Period. http://codereview.chromium.org/2873016/diff/24001/9003#newcode118 cros_mark_as_stable.py:118: def __init__(self, package, commit_id=None): What's this? Remove? http://codereview.chromium.org/2873016/diff/24001/9003#newcode124 cros_mark_as_stable.py:124: self.package = package Indent same as """ (here and everywhere). See style. http://codereview.chromium.org/2873016/diff/24001/9003#newcode134 cros_mark_as_stable.py:134: VPrint('Looking for unstable ebuild for %s' % package) Per style, you need explicit documentation of the arguments: http://code.google.com/p/soc/wiki/PythonStyleGuide#Functions_and_Methods Here and other public methods in this file. http://codereview.chromium.org/2873016/diff/24001/9003#newcode149 cros_mark_as_stable.py:149: """ Period. http://codereview.chromium.org/2873016/diff/24001/9003#newcode184 cros_mark_as_stable.py:184: self.ebuild.current_revision + 1) Period. Do you mean "an ebuild"? Do you mean "and/or"? I'd stick with just "or" instead -- it's an inclusive or, rather than an exclusive or (http://en.wikipedia.org/wiki/And/or). http://codereview.chromium.org/2873016/diff/24001/9003#newcode189 cros_mark_as_stable.py:189: (self.ebuild.ebuild_path_no_version, new_ebuild_path) How about RevEBuild instead? Or BumpEBuildRevision? http://codereview.chromium.org/2873016/diff/24001/9003#newcode198 cros_mark_as_stable.py:198: else: What does this mean? return True or return False? Also, the doc string is incomplete in this regard. http://codereview.chromium.org/2873016/diff/24001/9003#newcode204 cros_mark_as_stable.py:204: # Always add new commit_id after EAPI definition. Remove blank line. http://codereview.chromium.org/2873016/diff/24001/9003#newcode266 cros_mark_as_stable.py:266: if gflags.FLAGS.push: I'd just call the check method and let it Die inside. http://codereview.chromium.org/2873016/diff/24001/9003#newcode289 cros_mark_as_stable.py:289: Can you move this to PushChange? I.e., for now the PushChange method can simply print and return. http://codereview.chromium.org/2873016/diff/24001/9004 File cros_mark_as_stable_unittest.py (right): http://codereview.chromium.org/2873016/diff/24001/9004#newcode7 cros_mark_as_stable_unittest.py:7: """Unit tests for cros_mark_as_stable.py""" Period. http://codereview.chromium.org/2873016/diff/24001/9004#newcode25 cros_mark_as_stable_unittest.py:25: self.branch = 'test_branch' _branch. http://codereview.chromium.org/2873016/diff/24001/9004#newcode44 cros_mark_as_stable_unittest.py:44: branch.Delete() No need to stub Delete out? How did this work? http://codereview.chromium.org/2873016/diff/24001/9004#newcode68 cros_mark_as_stable_unittest.py:68: branch._initialized = True Why is this different than the rest of the tests? Can't you invoke Delete directly? http://codereview.chromium.org/2873016/diff/24001/9004#newcode69 cros_mark_as_stable_unittest.py:69: branch.branch_name = self.branch Isn't this set by the constructor? http://codereview.chromium.org/2873016/diff/24001/9004#newcode119 cros_mark_as_stable_unittest.py:119: cros_mark_as_stable.RunCommand(mox.StrContains( You still have a few of these StrContains present. Can you convert them to exact string matches instead? http://codereview.chromium.org/2873016/diff/24001/9004#newcode134 cros_mark_as_stable_unittest.py:134: # Test with ebuild without revision number. Separate test. http://codereview.chromium.org/2873016/diff/24001/9007 File generate_test_report.py (right): http://codereview.chromium.org/2873016/diff/24001/9007#newcode1 generate_test_report.py:1: #!/usr/bin/python This seems like it belongs to a separate CL. Either way, you also need to update crosutils.git/archive_hwqual which includes this script into the hwqual package.
PTAL :) http://codereview.chromium.org/2873016/diff/24001/9003 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/24001/9003#newcode19 cros_mark_as_stable.py:19: import generate_test_report On 2010/07/05 06:28:27, petkov wrote: > This seems unexpected. Add some comment why you're including this and maybe a > TODO to refactor into a separate common library. Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode23 cros_mark_as_stable.py:23: 'Board for which the package belongs', short_name='b') On 2010/07/05 06:28:27, petkov wrote: > Period. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode25 cros_mark_as_stable.py:25: 'Optional list of commit ids for each package.', On 2010/07/05 06:28:27, petkov wrote: > ... of space-separated list... > > What if it's not supplied? > > Also, the number of commit id's must match the number of packages. > > All seems worth documenting. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode28 cros_mark_as_stable.py:28: 'Marking 9999 ebuild for %s with commit %s as stable.', Simpler just removing the option. I don't think it's necessary. On 2010/07/05 06:28:27, petkov wrote: > %s's here appear in the usage, right? A bit ugly. > > Maybe put the default message format string in a separate variable and for the > usage use <package> and <commit_id> in place of %s. > http://codereview.chromium.org/2873016/diff/24001/9003#newcode40 cros_mark_as_stable.py:40: # TODO(sosa): Remove hard-coding of overlays directory once we find a better way. On 2010/07/05 06:28:27, petkov wrote: > 80 chars. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode56 cros_mark_as_stable.py:56: print 'Please specify at least one package' On 2010/07/05 06:28:27, petkov wrote: > Die? Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode59 cros_mark_as_stable.py:59: print 'Please specify a board' On 2010/07/05 06:28:27, petkov wrote: > Die? > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode62 cros_mark_as_stable.py:62: print 'Package list is not the same length as the commit id list' On 2010/07/05 06:28:27, petkov wrote: > Die? > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode69 cros_mark_as_stable.py:69: generate_test_report.Die('Usage: %s ARGS\n%s' % (sys.argv[0], gflags.FLAGS)) Let's leave as red as it is an error. On 2010/07/05 06:28:27, petkov wrote: > It might sound like I'm changing my mind here but it might seem strange to print > the whole usage in red. > http://codereview.chromium.org/2873016/diff/24001/9003#newcode83 cros_mark_as_stable.py:83: """Wrapper class for a git branch. """ On 2010/07/05 06:28:27, petkov wrote: > Remove space at the end. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode85 cros_mark_as_stable.py:85: def __init__(self, branch_name): On 2010/07/05 06:28:27, petkov wrote: > You probably need to document this as a public method. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode87 cros_mark_as_stable.py:87: self._initialized = False On 2010/07/05 06:28:27, petkov wrote: > _initialized is unused. Remove. > > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode92 cros_mark_as_stable.py:92: if not self._cleaned_up: On 2010/07/05 06:28:27, petkov wrote: > Can you call CleanUp here instead? > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode92 cros_mark_as_stable.py:92: if not self._cleaned_up: On 2010/07/05 06:28:27, petkov wrote: > Can you call CleanUp here instead? > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode95 cros_mark_as_stable.py:95: def Init(self): On 2010/07/05 06:28:27, petkov wrote: > Do you think Init is the right name given the functionality? Maybe it is, just > double checking... > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode100 cros_mark_as_stable.py:100: self._initialized = True On 2010/07/05 06:28:27, petkov wrote: > _initialized is unused. Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode102 cros_mark_as_stable.py:102: def CleanUp(self): On 2010/07/05 06:28:27, petkov wrote: > Missing doc string for a public method. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode107 cros_mark_as_stable.py:107: """Function used internally to create and move between branches""" On 2010/07/05 06:28:27, petkov wrote: > Period. Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode115 cros_mark_as_stable.py:115: """Returns True if the branch exists""" On 2010/07/05 06:28:27, petkov wrote: > Period. Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode118 cros_mark_as_stable.py:118: re.match On 2010/07/05 06:28:27, petkov wrote: > What's this? Remove? > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode124 cros_mark_as_stable.py:124: Returns True on success. On 2010/07/05 06:28:27, petkov wrote: > Indent same as """ (here and everywhere). See style. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode134 cros_mark_as_stable.py:134: def __init__(self, package, commit_id=None): Yeah sorry about that. Had them all lined up but didn't realign them after deleting the spaces before and after the """'s On 2010/07/05 06:28:27, petkov wrote: > Per style, you need explicit documentation of the arguments: > > http://code.google.com/p/soc/wiki/PythonStyleGuide#Functions_and_Methods > > Here and other public methods in this file. > http://codereview.chromium.org/2873016/diff/24001/9003#newcode149 cros_mark_as_stable.py:149: """Static method that returns the full path of an ebuild""" On 2010/07/05 06:28:27, petkov wrote: > Period. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode184 cros_mark_as_stable.py:184: """Class that will rev and ebuild and commit and | or push the rev change""" On 2010/07/05 06:28:27, petkov wrote: > Period. > > Do you mean "an ebuild"? > > Do you mean "and/or"? I'd stick with just "or" instead -- it's an inclusive or, > rather than an exclusive or (http://en.wikipedia.org/wiki/And/or). > > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode189 cros_mark_as_stable.py:189: def UpdateEBuild(self, commit_id, redirect_file=None): On 2010/07/05 06:28:27, petkov wrote: > How about RevEBuild instead? Or BumpEBuildRevision? > Good call, done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode198 cros_mark_as_stable.py:198: return On 2010/07/05 06:28:27, petkov wrote: > What does this mean? return True or return False? > > Also, the doc string is incomplete in this regard. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode204 cros_mark_as_stable.py:204: On 2010/07/05 06:28:27, petkov wrote: > Remove blank line. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode266 cros_mark_as_stable.py:266: if not _CheckSaneArguments(package_list, commit_id_list): On 2010/07/05 06:28:27, petkov wrote: > I'd just call the check method and let it Die inside. > Done. http://codereview.chromium.org/2873016/diff/24001/9003#newcode289 cros_mark_as_stable.py:289: print 'Push currently not implemented' On 2010/07/05 06:28:27, petkov wrote: > Can you move this to PushChange? I.e., for now the PushChange method can simply > print and return. > Done. http://codereview.chromium.org/2873016/diff/24001/9004 File cros_mark_as_stable_unittest.py (right): http://codereview.chromium.org/2873016/diff/24001/9004#newcode7 cros_mark_as_stable_unittest.py:7: """Unit tests for cros_mark_as_stable.py""" On 2010/07/05 06:28:27, petkov wrote: > Period. > Done. http://codereview.chromium.org/2873016/diff/24001/9004#newcode25 cros_mark_as_stable_unittest.py:25: self.branch = 'test_branch' On 2010/07/05 06:28:27, petkov wrote: > _branch. > Done. http://codereview.chromium.org/2873016/diff/24001/9004#newcode44 cros_mark_as_stable_unittest.py:44: branch.Delete() On 2010/07/05 06:28:27, petkov wrote: > No need to stub Delete out? How did this work? > Not sure why it didn't complain. Added stub. Must be related to the logic that everything that delete calls is already stubbed out. http://codereview.chromium.org/2873016/diff/24001/9004#newcode68 cros_mark_as_stable_unittest.py:68: branch._initialized = True Delete isn't static. On 2010/07/05 06:28:27, petkov wrote: > Why is this different than the rest of the tests? Can't you invoke Delete > directly? > http://codereview.chromium.org/2873016/diff/24001/9004#newcode69 cros_mark_as_stable_unittest.py:69: branch.branch_name = self.branch On 2010/07/05 06:28:27, petkov wrote: > Isn't this set by the constructor? Done. http://codereview.chromium.org/2873016/diff/24001/9004#newcode134 cros_mark_as_stable_unittest.py:134: # Test with ebuild without revision number. On 2010/07/05 06:28:27, petkov wrote: > Separate test. > Done. http://codereview.chromium.org/2873016/diff/24001/9007 File generate_test_report.py (right): http://codereview.chromium.org/2873016/diff/24001/9007#newcode1 generate_test_report.py:1: #!/usr/bin/python On 2010/07/05 06:28:27, petkov wrote: > This seems like it belongs to a separate CL. Either way, you also need to update > crosutils.git/archive_hwqual which includes this script into the hwqual package. > Done.
A few remaining issues, LGTM otherwise. You might want to get msb@ to review the functionality as well. http://codereview.chromium.org/2873016/diff/29001/30002 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/29001/30002#newcode69 cros_mark_as_stable.py:69: return True Remove the return. http://codereview.chromium.org/2873016/diff/29001/30002#newcode141 cros_mark_as_stable.py:141: Uses equery to find the ebuild path and sets data about an ebuild for You docstrings are still off, I think -- in terms of indentation and doc completeness (args, results, etc.) http://codereview.chromium.org/2873016/diff/29001/30002#newcode212 cros_mark_as_stable.py:212: for line in fileinput.input(new_ebuild_path, inplace=1): Try assigning to a temp before you start the loop and move stdout out. http://codereview.chromium.org/2873016/diff/29001/30002#newcode278 cros_mark_as_stable.py:278: if not work_branch.Exists(): It seems that CreateBranch should return True/False as a success status. http://codereview.chromium.org/2873016/diff/29001/30002#newcode296 cros_mark_as_stable.py:296: print 'An exception occurred %s' % e Maybe print a list of successful and unsuccessful packages on exception?
Petkov: addressed comments and fixed all public doc strings. Mandeep: Can you take another look please? http://codereview.chromium.org/2873016/diff/29001/30002 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/29001/30002#newcode69 cros_mark_as_stable.py:69: return True On 2010/07/08 23:02:19, petkov wrote: > Remove the return. > Done. http://codereview.chromium.org/2873016/diff/29001/30002#newcode141 cros_mark_as_stable.py:141: Uses equery to find the ebuild path and sets data about an ebuild for On 2010/07/08 23:02:19, petkov wrote: > You docstrings are still off, I think -- in terms of indentation and doc > completeness (args, results, etc.) > Done. http://codereview.chromium.org/2873016/diff/29001/30002#newcode212 cros_mark_as_stable.py:212: for line in fileinput.input(new_ebuild_path, inplace=1): On 2010/07/08 23:02:19, petkov wrote: > Try assigning to a temp before you start the loop and move stdout out. Had done and refactored, but it just doesn't work. My only conclusion is that it infact needs to read from fileinput.input before the direction actually works. Refactored some and you can see how could move the if above easily (tried but didn't work) so I'm leaving it there. http://codereview.chromium.org/2873016/diff/29001/30002#newcode278 cros_mark_as_stable.py:278: if not work_branch.Exists(): Prefer to keep the functionality separate for testability. On 2010/07/08 23:02:19, petkov wrote: > It seems that CreateBranch should return True/False as a success status. > http://codereview.chromium.org/2873016/diff/29001/30002#newcode296 cros_mark_as_stable.py:296: print 'An exception occurred %s' % e On 2010/07/08 23:02:19, petkov wrote: > Maybe print a list of successful and unsuccessful packages on exception? > Done.
http://codereview.chromium.org/2873016/diff/37001/38002 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/37001/38002#newcode88 cros_mark_as_stable.py:88: class _GitBranch(object): This abstraction seems unnecessary and doesn't really map well to the domain: what is a _GitBranch? Might be cleaner if you open coded this. http://codereview.chromium.org/2873016/diff/37001/38002#newcode168 cros_mark_as_stable.py:168: string, wihtout the version string, and the current revision number for s/wihtout/without/ http://codereview.chromium.org/2873016/diff/37001/38002#newcode223 cros_mark_as_stable.py:223: for line in fileinput.input(new_ebuild_path, inplace=1): Why not just read the 9999 and write the new file here instead of copying, editing in place and redirecting. Just mock the opening of the new file. http://codereview.chromium.org/2873016/diff/37001/38002#newcode297 cros_mark_as_stable.py:297: if not work_branch.Exists(): could this every happen. Wouldn't CreateBranch throw an exception if this is the case. http://codereview.chromium.org/2873016/diff/37001/38002#newcode299 cros_mark_as_stable.py:299: index = 0 why initialize this? http://codereview.chromium.org/2873016/diff/37001/38002#newcode300 cros_mark_as_stable.py:300: try: shouldn't this be higher up (right before the chdir) http://codereview.chromium.org/2873016/diff/37001/38002#newcode322 cros_mark_as_stable.py:322: # Always run the last two cleanup functions. Maybe this should be in a finally:
On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > http://codereview.chromium.org/2873016/diff/37001/38002 > File cros_mark_as_stable.py (right): > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode88 > cros_mark_as_stable.py:88: class _GitBranch(object): > This abstraction seems unnecessary and doesn't really map well to the domain: > what is a _GitBranch? Might be cleaner if you open coded this. > After discussion, this probably OK. > http://codereview.chromium.org/2873016/diff/37001/38002#newcode168 > cros_mark_as_stable.py:168: string, wihtout the version string, and the current > revision number for > s/wihtout/without/ > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode223 > cros_mark_as_stable.py:223: for line in fileinput.input(new_ebuild_path, > inplace=1): > Why not just read the 9999 and write the new file here instead of copying, > editing in place and redirecting. Just mock the opening of the new file. > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode297 > cros_mark_as_stable.py:297: if not work_branch.Exists(): > could this every happen. Wouldn't CreateBranch throw an exception if this is the > case. > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode299 > cros_mark_as_stable.py:299: index = 0 > why initialize this? > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode300 > cros_mark_as_stable.py:300: try: > shouldn't this be higher up (right before the chdir) > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode322 > cros_mark_as_stable.py:322: # Always run the last two cleanup functions. > Maybe this should be in a finally:
Thanks for taking another look msb. Incorporated changes. http://codereview.chromium.org/2873016/diff/37001/38002 File cros_mark_as_stable.py (right): http://codereview.chromium.org/2873016/diff/37001/38002#newcode88 cros_mark_as_stable.py:88: class _GitBranch(object): We talked. On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > This abstraction seems unnecessary and doesn't really map well to the domain: > what is a _GitBranch? Might be cleaner if you open coded this. Done. http://codereview.chromium.org/2873016/diff/37001/38002#newcode168 cros_mark_as_stable.py:168: string, wihtout the version string, and the current revision number for On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > s/wihtout/without/ Done. http://codereview.chromium.org/2873016/diff/37001/38002#newcode223 cros_mark_as_stable.py:223: for line in fileinput.input(new_ebuild_path, inplace=1): I prefer it because I only have to have one file to manage (open/close) rather than two. I know it makes it more clean for having a secondary file but the secondary file is mostly there only for mocking out purposes. On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > Why not just read the 9999 and write the new file here instead of copying, > editing in place and redirecting. Just mock the opening of the new file. http://codereview.chromium.org/2873016/diff/37001/38002#newcode297 cros_mark_as_stable.py:297: if not work_branch.Exists(): On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > could this every happen. Wouldn't CreateBranch throw an exception if this is the > case. True. Done. http://codereview.chromium.org/2873016/diff/37001/38002#newcode299 cros_mark_as_stable.py:299: index = 0 On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > why initialize this? Necessary to get scoping of the variable for the except http://codereview.chromium.org/2873016/diff/37001/38002#newcode300 cros_mark_as_stable.py:300: try: On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > shouldn't this be higher up (right before the chdir) No, we want to chdir to the overlays directory once. Another question is whether the try / except should be in / outside the for loop. I chose to fail the whole thing if one failed so kept the try outside. http://codereview.chromium.org/2873016/diff/37001/38002#newcode322 cros_mark_as_stable.py:322: # Always run the last two cleanup functions. On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: > Maybe this should be in a finally: Done.
ping On Fri, Jul 9, 2010 at 4:29 PM, <sosa@chromium.org> wrote: > Thanks for taking another look msb. Incorporated changes. > > > http://codereview.chromium.org/2873016/diff/37001/38002 > File cros_mark_as_stable.py (right): > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode88 > cros_mark_as_stable.py:88: class _GitBranch(object): > We talked. > > On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: >> >> This abstraction seems unnecessary and doesn't really map well to the > > domain: >> >> what is a _GitBranch? Might be cleaner if you open coded this. > > Done. > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode168 > cros_mark_as_stable.py:168: string, wihtout the version string, and the > current revision number for > On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: >> >> s/wihtout/without/ > > Done. > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode223 > cros_mark_as_stable.py:223: for line in fileinput.input(new_ebuild_path, > inplace=1): > I prefer it because I only have to have one file to manage (open/close) > rather than two. I know it makes it more clean for having a secondary > file but the secondary file is mostly there only for mocking out > purposes. > > On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: >> >> Why not just read the 9999 and write the new file here instead of > > copying, >> >> editing in place and redirecting. Just mock the opening of the new > > file. > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode297 > cros_mark_as_stable.py:297: if not work_branch.Exists(): > On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: >> >> could this every happen. Wouldn't CreateBranch throw an exception if > > this is the >> >> case. > > True. Done. > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode299 > cros_mark_as_stable.py:299: index = 0 > On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: >> >> why initialize this? > > Necessary to get scoping of the variable for the except > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode300 > cros_mark_as_stable.py:300: try: > On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: >> >> shouldn't this be higher up (right before the chdir) > > No, we want to chdir to the overlays directory once. Another question > is whether the try / except should be in / outside the for loop. I > chose to fail the whole thing if one failed so kept the try outside. > > http://codereview.chromium.org/2873016/diff/37001/38002#newcode322 > cros_mark_as_stable.py:322: # Always run the last two cleanup functions. > On 2010/07/09 21:25:20, Mandeep Singh Baines wrote: >> >> Maybe this should be in a finally: > > Done. > > http://codereview.chromium.org/2873016/show >
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
