Skip to content
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

Merged

Conversation

numerology
Copy link

@numerology numerology commented Oct 21, 2019

This is part of the efforts stated in #2442

The plan is as follows:

  1. 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?

  2. Make sure the configuration is acceptable to everyone.

  3. Sent out linting PRs to fix current formatting issues.

  4. Add pylint check to Travis CI.


This change is Reviewable

@neuromage
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 30a4ee4 into kubeflow:master Oct 21, 2019
@numerology numerology deleted the code-style-helper-and-enforcement branch October 21, 2019 19:46
@numerology
Copy link
Author

@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 split_before_first_argument = true

@Ark-kun
Copy link
Contributor

Ark-kun commented Oct 21, 2019

Thank you for doing this work!

@Ark-kun : the problem re: long-named function you mentioned once to me should be solved by split_before_first_argument = true

Great!

Please take a look and LMK if you have any comments/suggested changes.

One of my concerns is readability.
Python's standard library uses 4-space indents and I find it significantly more readable compared to python code formatted with 2-space indents. Indents are important in Python since there are no closing curly braces that help you see the depth. Thus, more prominent indentation makes the code more readable.

The official Python style guide is pretty clear here (explicit is better than implicit):
https://www.python.org/dev/peps/pep-0008/#indentation

Indentation

Use 4 spaces per indentation level.

I'll try to find some time and see how the rest of the options look when applied to the KFP code.

@numerology
Copy link
Author

I find it significantly more readable compared to python code formatted with 2-space indents.

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?

@Bobgy
Copy link
Contributor

Bobgy commented Oct 21, 2019

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?

@numerology Thanks! I will update about frontend part.

magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
Signed-off-by: Dan Sun <dsun20@bloomberg.net>

Signed-off-by: Dan Sun <dsun20@bloomberg.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants