-
Notifications
You must be signed in to change notification settings - Fork 568
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
bump yapf to 0.43.0 #4710
bump yapf to 0.43.0 #4710
Conversation
Signed-off-by: Javan Lacerda <javanlacerda@google.com>
@@ -1,5 +1,5 @@ | |||
[style] | |||
based_on_style = chromium | |||
based_on_style = google |
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.
Let's leave this as is for now. I don't want to pollute blame history with formatting changes.
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.
Hmm, but this is blocking this PR #4707. The current version doesn't support the match case
usage. However I can modify the PR to use if else
instead, and avoid this bump. WDYT?
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.
Chromium doesn't support match case
? Bizzare.
I guess land this then.
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 doesn't in the version 0.22 ;/. The version 0.29.1 supports but the lint behavior is different as well.
.style.yapf
Outdated
@@ -1,5 +1,5 @@ | |||
[style] | |||
based_on_style = google | |||
based_on_style = pep8 |
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.
Hmm...Why switch to this? I think this is a much bigger change, can we just switch to google?
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.
Actually I was just checking if it was going back to the initial state. This change log https://github.com/search?q=repo%3Agoogle%2Fyapf%20chromium&type=code says that the chromium
style was renamed to use the pep8
style
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.
But it seems that the behavior is different
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 reseted to the bump for 0.43.0 version.
45e282c
to
d192d8c
Compare
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.
Javan, on further thought I think it would be a bad idea to land this right now. Just get rid of the match case for now.
Landing this change will probably make it extremely difficult to merge the oss-fuzz branch back into the master branch.
UNBLOCKS #4707
It bumps the yapf library to 0.43.0
It's needed to support new python features, such as
match case
.I've run the following command to validate it's working properly:
yapf -p -i src/clusterfuzz/_internal/bot/fuzzers/centipede/engine.py src/clusterfuzz/_internal/bot/tasks/utasks/corpus_pruning_task.py src/clusterfuzz/_internal/tests/core/bot/fuzzers/centipede/centipede_engine_test.py src/clusterfuzz/_internal/tests/core/bot/tasks/utasks/corpus_pruning_task_test.py
The given files were formatted correctly following the new style
google
instead of the deprecated onechromium