<p><a href="https://gerrit.asterisk.org/c/repotools/+/10941">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10941/5/process-staging-changes">File process-staging-changes:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10941/5/process-staging-changes@18">Patch Set #5, Line 18:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> sce = StagingChangesExtractor(ast_path=ns.local_root, start_version=ns.start_version, end_version=ns.end_version)<br> ret = sce.run()<br><br> # Only add trailing slash for printing message; it's not needed otherwise<br> if not ns.local_root.endswith("/"):<br> ns.local_root += "/"<br><br> if ret is 0:<br> used_l_opt = ""<br> if ns.local_root != "/tmp/":<br> used_l_opt = "-l {0} ".format(ns.local_root)<br> print("Done! Check {0}asterisk and make sure everything looks right. "<br> "Then run 'commit-staging-changes {1}-v {2}'.".format(ns.local_root, used_l_opt, ns.end_version))<br> else:<br> print("Error! Check the logs and remember to clean up {0}asterisk".format(ns.local_root))<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Could this be extracted into it's own function and reused by mkreleasse. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think the only part that could be reused is instantiating the object, running it, and checking the return value. The rest of it is only useful for the script, imo. Could be an option if combining the commit part of the other script as an option to this.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py">File staging_changes.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py@52">Patch Set #5, Line 52:</a> <code style="font-family:monospace,monospace"> def __init__(self, ast_path, start_version, end_version):</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">a "repo" is not passed in, but just the path. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Passing in a repo could work (or rather, taking the path and checking to see if the repo exists or not). I'm not sure on checking out the repo though, since the intention of this script is to do work on an existing repository. I would lean towards continuing or exiting the script based on whether or not the repository exists at the path. This also fits with how mkrelease.py does its logic, leaving the repository checkout to it to get stuff ready for the script. Just my opinion, feel free to push back if you feel strongly about it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py@120">Patch Set #5, Line 120:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> with open(staging_path + "/" + filename, 'r') as f:<br> subject = ""<br> message = ""<br> master = False<br> line = f.readline()<br> while line != "":<br> # Get the subject<br> if line.startswith("Subject:"):<br> if subject != "":<br> # Since this is not the first subject, we have data to add<br> container = self.Container(subject, message, timestamp)<br> if master is True:<br> self.master_data.append(container)<br> else:<br> self.data[subject].append(container)<br> message = ""<br> subject = line.split("Subject:")[1].lstrip().rstrip()<br> line = f.readline()<br> if line.startswith("Master-Only:"):<br> val = line.split("Master-Only:")[1].lstrip().rstrip()<br> if "true" in val or "True" in val:<br> master = True<br> f.readline()<br> else:<br> master = False<br> line = f.readline()<br> message = " * " + line<br> elif line == "\n":<br> # This is a special case because if there is an empty line separating some<br> # text, we don't want to add 3 spaces and then a new line.<br> message += "\n"<br> else:<br> message += " " + line<br> line = f.readline()<br> container = self.Container(subject, message, timestamp)<br> if master is True:<br> self.master_data[subject].append(container)<br> else:<br> self.data[subject].append(container)<br> # Clean up the staging directory as we cycle through the files<br> LOGGER.debug("Removing file {0}".format(filename))<br> os.remove(os.path.join(staging_path, filename))<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">While this works, it's kinda "brittle". […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yeah I agree with this, I was giving it some thought earlier and it's not very easily maintainable in its current state. I'll look at ways to improve this.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10941/5/staging_changes.py@221">Patch Set #5, Line 221:</a> <code style="font-family:monospace,monospace"> def run(self):</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Another suggestion. If you have a repo after running you could go ahead and add and commit the code. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This could be an option (ties in to comment on other file), although I am hesitant to put something like that in the same script since an option like that can be missed and then we would have to deal with that, which is a pain. I like having another script (that could also be used for other purposes, if need be) that doesn't have to go through all the checks of running this one again if run the first time as a "prep".</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/repotools/+/10941">change 10941</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/repotools/+/10941"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: repotools </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I6dc084afedaeecaf36aaec66c3cf6a5a8ed4ef3c </div>
<div style="display:none"> Gerrit-Change-Number: 10941 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 28 Mar 2019 14:57:48 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>