[Asterisk-code-review] mkrelease: Add a Python script that creates tarballs for Ast... (repotools[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: mkrelease: Add a Python script that creates tarballs for Asterisk
......................................................................


Patch Set 3:

(26 comments)

https://gerrit.asterisk.org/#/c/383/3/mkrelease.py
File mkrelease.py:

Line 85:     if 'yes' in cont.lower() or 'y' in cont.lower():
> Blank line here.
I agree with your next one, not as much with this one. We've already delineated between the prompt and getting the input; the only input we parse is 'yes' or 'y', so an extra space between retrieving the raw_input and checking for that value feels extraneous.


Line 87:     raise ExitException()
> Blank line here.
Fixed.


Line 96: global version
       :     global version_object
       :     global release_name
       :     global interactive
       :     global debug
> Why not return an options object with these properties and use it going for
That's what I had originally. I then ended up passing options into every function, which got messy.

These really are 'global' properties - once determined, they are set for the lifetime of the script and treated as immutable items. Hence, getting them all set up once here and then used elsewhere simplified a lot of the function definitions.


Line 123:     return repo
> Blank line here.
I don't think a blank line is warranted before the return. Once before the dprint might be nice however.


Line 130:     options - Parsed command line arguments
> options doesn't appear to be there anymore.
See my previous comment about the refactor :-)

Removed.


Line 134: mainline = version[:version.index('.')]
        :     branch = version[:version.rindex('.')]
> Can any of this be fetched more easily from the version_object?
Probably. I'll tweak that up.


Line 136:     if repo.remote_branch_exists(branch):
> Blank line here.
I don't think that helps readability. We're merely warning that the branch exists already, a blank line won't make that any clearer.


Line 145:     repo.checkout_remote_branch(branch)
> Blank line here.
Fixed.


Line 147:     return
> Blank line here.
That doesn't help readability here.


Line 162: global prev_version
        :     global start_version
> Why not return a tuple of versions and use it going forward?
See previous comment about passing things around through all of the functions.


Line 166:     if len(version_object.modifiers) == 0 and version_object.patch == 0:
> Blank line here.
Fixed


Line 171:         while search:
> Blank line here.
This is another place I disagree. Python uses white space to format blocks, and placing any extra line in here actually appears to break the flow worse. This would similar to doing something like:

if (foo) {

   bar;

} else {

   yackity;

}

Or:

search = True

while (true) {
  ...
}

Which feels worse than:

search = True
while (true) {
    ...
}


Line 196:     if options.previous_tag:
> Blank line here.
These are both following the previous comment - they are overriding the options if explicit values were provided. Putting a blank line between the if statements implies that they are two separate sets of actions, which is really not the case.


Line 200: def create_tag(options, repo):
> Perhaps this function should be renamed to better reflect it's intended use
I'll take suggestions?

In the end, it does create the tag: all of it :-)  In Asterisk terms, that includes everything that is listed below.


Line 275:             if fnmatch.fnmatch(f_name, '{0}-*-summary.*'.format(project)):
> Blank line here.
Again, I actually think this makes it less clear what the code is doing.


Line 281:         if removed:
> Blank line here.
I don't think a newline would aid in readability here.


Line 317:         with open(ver_file_path, 'w') as ver_file:
> Blank line here.
This doesn't help readability.


Line 319:         dprint(".version file written as '{0}'".format(version))
> Blank line here.
A blank line before the dprint would help here.


Line 367:         return
> Blank line here.
See previous comment on placing a blank line before a 'return' statement.


Line 414:     if prev_obj.major != version_object.major:
> Blank line here.
I disagree. Getting a previous object logically precedes the test that uses the object.


Line 420:     # Since we don't want to work with the diff, but merely dump it out,
> Blank line here.
Fixed.


Line 437:     with tarfile.open(name=out_file, mode='w:gz') as tar_obj:
> Blank line here.
Placed before out_file, since that is used with the 'with' statement.


Line 460:     with open(out_file, 'w') as fs:
> Blank line here.
Placed before dprint


Line 462:     return out_file
> Blank line here.
I'm okay with that here, since we are returning something.


Line 515:         with tarfile.open(name=name, mode='w:gz') as tar_obj:
> Blank line here.
All of the previous statements build into the 'with' statement. I don't think breaking them into logical groupings works here.


Line 530:     return
> Blank line here.
See previous.


-- 
To view, visit https://gerrit.asterisk.org/383
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibab7643d501085bcb2aad13915d4b0f6b24cded4
Gerrit-PatchSet: 3
Gerrit-Project: repotools
Gerrit-Branch: master
Gerrit-Owner: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Samuel Galarneau <sgalarneau at digium.com>
Gerrit-HasComments: Yes