-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
I'd go as far as to remove all customisation from Black (i.e. no skipping of string normalisation and no custom line length). If we're going to use it, let's use it 🙂 |
I agree with using the default line length (see e092d47), but I disagree with the string "normalisation". We are way more often using double quotes inside single quotes (for JSON, for "highlighting", etc.) than the other way around. |
@ala-ableton that sounds fine to me. I've squashed and rebased your commits into the latest push (d0e98b6). |
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 rather see a message like “Pin pipenv version in Dockerfile” for 52fad94. Also, forgive my lazy lack of googling, but I assume that black will not recognize a shared setup.cfg
for the config file? If so, it would be nice to use that file rather than making a new one.
This is needed to fix hadolint error DL3013.
@nre-ableton unfortunately, |
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.
LGTM
@@ -14,4 +14,4 @@ pydocstyle = "*" | |||
pylint = "*" | |||
|
|||
[requires] | |||
python_version = "3.6" | |||
python_version = "3.7" |
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.
Shouldn't Pipfile.lock
also change in the same commit?
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.
Good catch, I'll update.
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.
Addressed as of 8f829c1.
jenkins_node_scanner.py
Outdated
@@ -86,7 +77,7 @@ def get_args(): | |||
dest='urls', | |||
required=True, | |||
help='Full URL to the Jenkins master, such as http://jenkins:80. May be given ' | |||
'multiple times for multiple masters.', | |||
'multiple times for multiple masters.', |
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.
It seems that I forgot this when I made my fixup.
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.
Addressed in a7cba53.
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.
LGTM 👍
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.
LGTM 👍
@AbletonDevTools/gotham-city ptal!