-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[Request for comments] Add config for yapf and pylintrc #2446
[Request for comments] Add config for yapf and pylintrc #2446
Conversation
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neuromage The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@hongye-sun @SinaChavoshi @Ark-kun @gaoning777 Please take a look and LMK if you have any comments/suggested changes. @Ark-kun : the problem re: long-named function you mentioned once to me should be solved by |
Thank you for doing this work!
Great!
One of my concerns is readability. The official Python style guide is pretty clear here (explicit is better than implicit):
I'll try to find some time and see how the rest of the options look when applied to the KFP code. |
Either way looks good to me. However I imagine that once the unified DSL happens, at least the dsl part of the repo will be enforced 2-space indents, and if we do 4-space indents now, we'll have a lot of friction at that time. WDYT? |
@numerology Thanks! I will update about frontend part. |
Signed-off-by: Dan Sun <dsun20@bloomberg.net> Signed-off-by: Dan Sun <dsun20@bloomberg.net>
This is part of the efforts stated in #2442
The plan is as follows:
Make it clear in contributing guidance that we chose google python style and help contributors correctly configure their IDE to adopt it smoothly. @Bobgy can you help update the frontend part?
Make sure the configuration is acceptable to everyone.
Sent out linting PRs to fix current formatting issues.
Add pylint check to Travis CI.
This change is