-
Notifications
You must be signed in to change notification settings - Fork 4
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
Format with black #28
Conversation
Yeah good idea, let's merge it |
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 formatter definitely improves the code quality. That said, I think it fails in certain places as I outlined.
self.ml_model = LinearRegressionModel(self.config["function_name"], self.config["vendor"], self.config["DB_URL"], self.config["DB_PORT"], self.config["last_log_timestamp"], benchmark_dir)#TODO: Parametrize ML model constructor with factory method | ||
# Instantiate SPOT system components | ||
self.price_retriever = AWSPriceRetriever( | ||
self.config["DB_URL"], self.config["DB_PORT"], self.config["region"] |
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.
There is an inconsistency here compared to below multi-parameter object instantiatations.
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's based on line length. These three can all fit on the same line (just not the line with the assignment), so it leaves them that way, but the others don't, so they get split up.
def __init__(self, host, stage, resource, service='execute-api', content='application/x-amz-json-1.0') -> None: | ||
self.access_key = os.environ.get('AWS_ACCESS_KEY_ID') | ||
self.secret_key = os.environ.get('AWS_SECRET_ACCESS_KEY') | ||
def __init__( |
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 don't find this multi-line parameter formatting useful for easier code reading
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 does this when a line is too long. I don't think it's as good as having everything one one line if it's only a few parameters, but to me it seems more readable than one very long line.
payload_hash = hashlib.sha256(payload.encode('utf-8')).hexdigest() | ||
canonical_uri = '/' + self.stage + '/' + self.resource | ||
canonical_request = method + '\n' +canonical_uri +'\n' + canonical_querystring + '\n' + canonical_headers + '\n' + signed_headers + '\n' + payload_hash | ||
canonical_querystring = "" |
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 here.
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 actually don't think this line was very readable either way; it would probably be better as a multiline f-string like this:
f"""
{method}
{canonical_uri}
{canonical_querystring}
...
"""
(there might be some whitespace issues in my example)
But it's kind of moot since this code is all removed in #27.
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.
Although we have a disagreement in certain points on best practices, I approve the changes. The more important thing is that we have unified coding format throughout the code base and black achieves this.
Formatting all our code with
black
gives us easy, consistent styling. If we want to adopt it, I can set up an action to apply it to every PR automatically.