Fix trailing comma when no GitHub user is found #126
Conversation
Also: * Support disabling mapping to a GitHub user on the command line, for incorrectly-mapped BB users without a known GH account. * Clarify help text of --skip-attribution-for * Update help text in README with some recently added flags
| attributed. Useful to skip your own comments, because | ||
| you are running this script, and the GitHub comments | ||
| will be already under your name. | ||
| Your BitBucket username, if you don't want comments |
There was a problem hiding this comment.
Hmm... been a bit since I've looked at this code, but I thought this isn't just for your bb username... it can be applied to any username?
Also, your updated comment for --map-user suggests that this flag may not even be needed? Since you can just map your own name to an empty value? Is that correct?
There was a problem hiding this comment.
The code as it stands only really works for your username, particularly since the issue template attributes the issue to "me". Even if the template was changed, though, it doesn't really make sense to pass any other user, since it can only be set once and without attribution the comment effectively is being attributed to the user running the script.
| else: | ||
| gh_user = "" | ||
| return (user['display_name'] + " (" + bb_user + ", " + gh_user + ")") | ||
| return "{0} ({1}{2})".format(user['display_name'], bb_user, gh_user) |
There was a problem hiding this comment.
Can we use the nifty new f strings on this one? To make it more readable?
There was a problem hiding this comment.
Happy to; let me know your thoughts on your other comments and I'll update this PR.
| @@ -658,10 +662,10 @@ def format_user(user, options): | |||
| bb_user = "Bitbucket: [{0}](https://bitbucket.org/{0})".format(user['nickname']) | |||
| gh_username = _gh_username(user['nickname'], options.users, options.gh_auth) | |||
| if gh_username is not None: | |||
| gh_user = "GitHub: [{0}](https://github.com/{0})".format(gh_username) | |||
| gh_user = ", GitHub: [{0}](https://github.com/{0})".format(gh_username) | |||
There was a problem hiding this comment.
I am not understanding the impact of this... Can you explain / provide example of the bug that this solves?
There was a problem hiding this comment.
In the current code, you'd get this behavior (simplified example):
>>> bb_user = "MyUsername"
>>> gh_username = None
>>> if gh_username is not None:
... gh_user = "GitHub: [{0}](https://github.com/{0})".format(gh_username)
... else:
... gh_user = ""
...
>>> print(" (" + bb_user + ", " + gh_user + ")")
(MyUsername, )Note the trailing comma. This change ensures the comma separator is only added if gh_user will be non-empty.
There are other options, e.g. something like:
usernames=[bb_user]
if gh_username is not None:
usernames.append("GitHub: [{0}](https://github.com/{0})".format(gh_username))
print(" (" + ', '.join(usernames) + ")")If you prefer that, but this case seemed simple enough to just move the location of the comma.
| @@ -613,7 +615,9 @@ def format_change_element(change_element): | |||
|
|
|||
| def _gh_username(username, users, gh_auth): | |||
| try: | |||
| return users[username] | |||
| user = users[username] | |||
| return user if user else None # No GH account for this user | |||
There was a problem hiding this comment.
Would user = users.get(username) be the same thing?
And why do this? We're already catching the KeyError and passing...
There was a problem hiding this comment.
We're explicitly converting a user-provided empty-string to None and short-circuiting, to allow the caller to exclude usernames that shouldn't be mapped to GH. Falling through via pass would wind up in the auto-associate logic instead.
The goal is to let the caller disable the auto-associate behavior if it's wrong. If a BB user has a name that also exists on GH, but isn't the same person, there's no way currently to disable that mapping.
|
@jeffwidman what are next steps here? The trailing comma behavior is clearly a bug, if there's other changes in this PR you're unsure about let me know and I can pull them out into a separate PR. |
Also: