-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Changed formatted strings to fstrings in add_collaborators.py #24
base: BoatswainFormatToFString
Are you sure you want to change the base?
Changed formatted strings to fstrings in add_collaborators.py #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments - Thanks Jake!
opt.warn('Aborting') | ||
return | ||
|
||
opt.info('Proceeding to add {} as collaborators to {} with {} permissions' | ||
.format(opt.users.name, repo_full_name, opt.permission)) | ||
opt.info(f'Proceeding to add {opt.users.name} as collaborators to {repo_full_name} with {opt.permission} permissions' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a closing parentheses
+ f'Boatswain configuration ({config_path}). Please add this information ' | ||
+ 'to your config file under section [canvas] with key "token".' | ||
+ '\n'.format(config_path)) | ||
+ '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need the +
- Python strings are implicitly concatenated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the rest of the changes in this file
else: | ||
promptf = prompt | ||
|
||
promptf = '{}: '.format(promptf) | ||
promptf = '{promptf}: ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a starting f
'for users in {} under org {} with name {}') | ||
.format(opt.groups.name, opt.org, | ||
fmt_hyphen(opt.prefix, '<group>')), | ||
f'for users in {opt.groups.name} under org {opt.org} with name {fmt_hyphen(opt.prefix, '<group>')}'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would split this into multiple lines. Also, I'm not sure the quotes surrounding '<group>'
are going to work. Quotes in the format part of f-strings can get tricky - I would test that and see if it potentially works with double quotes. The cleanest solution may just be to turn the fmt_hyphen
section into a variable and use that instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for line 57
Also, if you could edit the name of the PR to match the rest of the changes made that would be great |
Changed the formatted strings in add_collaborators to fstrings