Skip to content

blurb: get BPO# from branch, add commit template #184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

blurb: get BPO# from branch, add commit template #184

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 43 additions & 18 deletions blurb/blurb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
#
# Please enter the relevant bugs.python.org issue number here:
#
.. bpo:
.. bpo: {bpo}

#
# Uncomment one of these "section:" lines to specify which section
Expand Down Expand Up @@ -838,24 +838,14 @@ def add():
os.close(handle)
atexit.register(lambda : os.unlink(tmp_path))

def init_tmp_with_template():
with open(tmp_path, "wt", encoding="utf-8") as f:
# hack:
# my editor likes to strip trailing whitespace from lines.
# normally this is a good idea. but in the case of the template
# it's unhelpful.
# so, manually ensure there's a space at the end of the bpo line.
text = template

bpo_line = ".. bpo:"
without_space = "\n" + bpo_line + "\n"
with_space = "\n" + bpo_line + " \n"
if without_space not in text:
sys.exit("Can't find BPO line to ensure there's a space on the end!")
text = text.replace(without_space, with_space)
f.write(text)
# see of the branch name indicated BPO number
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize English is not your first language; may I propose "see if the branch name indicates the BPO number"?

Copy link
Member

@terryjreedy terryjreedy May 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our current PEP 8 standard for stdlib comments is to make them complete sentences, with initial cap and period, as in
# See if the branch name indicates the BPO number.
Many old comments do not meet this standard, but new or edited ones should
-- unless Larry as blurb author disagrees ;-).

# makes the 'editor strips trailing whitespace from template lines'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that your patch obviates (and subsequently removes) the hack in question, this two-line comment is meaningless.

# hack obsolete, too.
branch_bpo, branch_suffix = get_bpo_git_branch()

init_tmp_with_template()
with open(tmp_path, "wt", encoding="utf-8") as f:
text = template.format(bpo=branch_bpo)
f.write(text)

while True:
subprocess.run([editor, tmp_path])
Expand Down Expand Up @@ -888,6 +878,18 @@ def init_tmp_with_template():
path = blurb.save_next()
git_add_files.append(path)
flush_git_add_files()

# For convenience, write a preformatted commit template. Enable template
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, a minor suggestion regarding your English here. The first sentence is fine, for the second may I suggest "You can tell git to use this template with "git config commit.template .git/blurb"."

It's not a big deal, but I don't know why you use `` to quote the command here. Simple double-quotes are fine, and if you want something clearer I would indent and put a % to indicate a command prompt.

Also, based on the code, it looks like

  1. this only works if the ~/.git/blurb directory exists, and
  2. blurb doesn't create the directory for you automatically.

Does git create the directory for you? If not, we should probably suggest that too in the comment, or else people will follow the instructions only to discover that they don't work.

# with ``git config commit.template .git/blurb``.
msgfile = os.path.join(root, '.git', 'blurb')
if os.path.isdir(os.path.dirname(msgfile)):
metadata, text = blurb[0]
title = branch_suffix.replace('-', ' ').replace('_', ' ')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. You are replacing all dashes and all underscores with spaces here. This means that a branch called "bpo-1234-Fix-__dir__-on-dict" would be unhelpfully converted into the template " bpo-1234: Fix dir on dict" with three spaces on either side of dict. (GitHub apparently refuses to display the three spaces,
and collapses them to one during formatting.)

If we honor the name of the branch, we should honor the name of the branch. There are legitimate uses for underscores and dashes, and stripping them all out is more surprising than simply leaving them there. I realize git doesn't permit spaces in branch names, so we can't simply write a wonderful commit message in the branch name and have it emerge from the other end of the process perfectly. Ultimately, leaving the dashes and underscores there may be a little inelegant but that's better than outright corrupted.

Please remove this line and resubmit the PR.

with open(msgfile, 'wt', encoding="utf-8") as f:
f.write(f"bpo-{metadata['bpo']}: {title}\n")
f.write("\n")
f.write(text)

print("Ready for commit.")


Expand Down Expand Up @@ -1067,6 +1069,29 @@ def flush_git_rm_files():
git_rm_files.clear()


def get_bpo_git_branch():
"""Try to detect bpo number from branch name

Supports:
bpo1234
bpo_1234
bpo-1234

returns: branch number, branch suffix
"""
try:
p = subprocess.run(["git", "rev-parse", "--abbrev-ref", "HEAD"], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
p.check_returncode()
except subprocess.CalledProcessError:
return '', ''
branch = p.stdout.decode('utf-8').strip()
mo = re.match("^bpo[-_]?(\d+)[-_]?(.*)", branch)
if mo is not None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eschew The Unnecessary Else!

I propose you rewrite these four lines as:

   if mo is None:
      return '', branch
   return mo.group(1), mo.group(2)

return mo.group(1), mo.group(2)
else:
return '', branch


# @subcommand
# def noop():
# "Do-nothing command. Used for blurb smoke-testing."
Expand Down