<p style="white-space: pre-wrap; word-wrap: break-word;">Mostly just a few suggestions. Feel free to ignore. However, -1 as I do feel like moving the parsing to it's own function is worthwhile since we'll need to do validation.</p><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 style="white-space: pre-wrap; word-wrap: break-word;">Could this be extracted into it's own function and reused by mkreleasse.py? (might not make sense, but could be worth consolidating code)</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 style="white-space: pre-wrap; word-wrap: break-word;">a "repo" is not passed in, but just the path. What if the path exists, but is not a valid Asterisk repo? I'd suggest passing in a repo object, and if 'None' then create and checkout the repo.</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 style="white-space: pre-wrap; word-wrap: break-word;">While this works, it's kinda "brittle". Meaning it's not conducive to additions and makes it so the key:value pairs are order dependent. I'd suggest removing the ordering dependency as well as breaking most of this functionality out into it's own function. We'll want to use the same code to check if the file is validly formatted at review time, so we don't run into broken files at release time.</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 style="white-space: pre-wrap; word-wrap: break-word;">Another suggestion. If you have a repo after running you could go ahead and add and commit the code. If needed you pass in a flag for times when you don't want to do that.</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: Wed, 27 Mar 2019 23:05:32 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>