default discussion title if not provided#271
Conversation
|
|
||
| if args.Input.Title == nil { | ||
| // Title defaults to first line of contents. | ||
| title := strings.TrimSpace(strings.SplitN(args.Input.Contents, "\n", 2)[0]) |
There was a problem hiding this comment.
args.Input.Contents can start with a bunch of whitespace, in which case we should ignore it:
>
>
> my title is here
>
> and my comment..
There was a problem hiding this comment.
I do recognize that:
strings.SplitN(args.Input.Contents, "\n", 2)[0]is more efficient than this:
strings.Split(args.Input.Contents, "\n")[0]But I think the latter is far more readable and we do not need to care about performance here. For example, I had to look up the exact semantics of the SplitN function to determine that the behavior here is correct.
There was a problem hiding this comment.
I put the trim before the split, which should solve the first issue.
As far as SplitN goes, I am ambivalent. I am all for readability, and it does require looking at the doc if you don't know it off the top of your head, but I think that is ok. The doc here isn't complicated, and it is a standard library function. Performance doesn't matter that much here, but it is unbounded user input. It seems silly to do something suboptimal when the optimal thing is a few keystrokes away.
Since my name is going to be on the blame, I am going to leave it as-is. You can push a commit after I squash merge if you feel strongly about it.
Codecov Report
|
|
LGTM, thanks! |
4 participants
see sourcegraph/sourcegraph-vscode-DEPRECATED#19 (comment)