Default --title on --message and clamp at 100 chars (issue 5574050)

[mar...@chromium.org]

Reviewers: techtonik, Andi Albrecht,

Message:
It's more or less what chromium's wrapper tools are doing.

I thought about using "…" instead of "...". I'm just afraid of breaking
non utf-8 compatible workflows. (Note that it's not a bad idea to break
broken tools)

Description:
Default --title on --message and clamp at 100 chars

Please review this at http://codereview.appspot.com/5574050/

Affected files:
M upload.py


Index: upload.py
===================================================================
--- a/upload.py
+++ b/upload.py
@@ -2205,15 +2205,12 @@
prompt = "Title describing this patch set: "
else:
prompt = "New issue subject: "
- title = options.title or raw_input(prompt).strip()
- if not title:
- ErrorExit("A non-empty title is required")
rpc_server = GetRpcServer(options.server,
options.email,
options.host,
options.save_cookies,
options.account_type)
- form_fields = [("subject", title)]
+ form_fields = []

repo_guid = vcs.GetGUID()
if repo_guid:
@@ -2238,19 +2235,33 @@
for cc in options.cc.split(','):
CheckReviewer(cc)
form_fields.append(("cc", options.cc))
- message = options.message
+
+ # Process --message, --title and --file.
+ message = options.message or ""
+ title = options.title or ""
if options.file:
if options.message:
ErrorExit("Can't specify both message and message file options")
file = open(options.file, 'r')
message = file.read()
file.close()
+ title = (
+ title or message.split('\n', 1)[0].strip() or
raw_input(prompt)).strip()
+ if not title:
+ ErrorExit("A non-empty title is required")
+ if len(title) > 100:
+ title = title[:97] + '...'
+ if title and not options.issue:
+ message = message or title
+
+ form_fields.append(("subject", title))
if message:
if not options.issue:
form_fields.append(("description", message))
else:
- # TODO: [ ] figure out how to send a comment from upload.py
+ # TODO: [ ] Use /<issue>/publish to add a comment.
pass
+
# Send a hash of all the base file so the server can determine if a copy
# already exists in an earlier patchset.
base_hashes = ""

[albrec...@googlemail.com]

LGTM

On 2012/01/23 19:39:12, M-A wrote:
> It's more or less what chromium's wrapper tools are doing.

> I thought about using "…" instead of "...". I'm just afraid of
breaking non
> utf-8 compatible workflows. (Note that it's not a bad idea to break
broken
> tools)

+1 for breaking them - yes, I want to use Ümläüts :)

http://codereview.appspot.com/5574050/

[albrec...@googlemail.com]

sorry, I've had two comments - but just nits, so it's still a LGTM

http://codereview.appspot.com/5574050/diff/2001/upload.py
File upload.py (right):

http://codereview.appspot.com/5574050/diff/2001/upload.py#newcode2207
upload.py:2207: prompt = "New issue subject: "
What about moving the prompt definition closer to raw_input() again.
AFAICT it is only used for this purpose.

http://codereview.appspot.com/5574050/diff/2001/upload.py#newcode2249
upload.py:2249: title or message.split('\n', 1)[0].strip() or
raw_input(prompt)).strip()
Indent 2 spaces

http://codereview.appspot.com/5574050/

[mar...@chromium.org]

Now with ÜTF-8 requirement.

http://codereview.appspot.com/5574050/diff/2001/upload.py
File upload.py (right):

http://codereview.appspot.com/5574050/diff/2001/upload.py#newcode2249
upload.py:2249: title or message.split('\n', 1)[0].strip() or
raw_input(prompt)).strip()

On 2012/01/23 19:52:43, Andi Albrecht wrote:
> Indent 2 spaces

Why? It's like a function call, no?

http://codereview.appspot.com/5574050/diff/4002/upload.py
File upload.py (right):

http://codereview.appspot.com/5574050/diff/4002/upload.py#newcode2254
upload.py:2254: title = title[:99] + '…'
I'm actually shocked that it worked on the first time. Added encoding at
the top of the file.

http://codereview.appspot.com/5574050/

[albrec...@googlemail.com]

http://codereview.appspot.com/5574050/diff/2001/upload.py
File upload.py (right):

http://codereview.appspot.com/5574050/diff/2001/upload.py#newcode2249
upload.py:2249: title or message.split('\n', 1)[0].strip() or
raw_input(prompt)).strip()

I'm not sure about the correct indentation level here. 2 spaces seems to
fit fine with the rest of the file.