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

Standardize code style using Black #1284

Closed
wants to merge 12 commits into from
Closed

Conversation

deepyaman
Copy link
Contributor

@deepyaman deepyaman commented Feb 16, 2020

Close #755

TODO:

  • Add pre-commit config (testing build first)
  • Pare down list of formatted files, if necessary
  • Document pre-commit

@deepyaman deepyaman changed the title Standardize code style using Black [WIP] Standardize code style using Black Feb 16, 2020
@codecov-io
Copy link

codecov-io commented Feb 16, 2020

Codecov Report

Merging #1284 into master will increase coverage by 2.33%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1284      +/-   ##
==========================================
+ Coverage   92.77%   95.11%   +2.33%     
==========================================
  Files          34       34              
  Lines        7266     7266              
==========================================
+ Hits         6741     6911     +170     
+ Misses        525      355     -170
Impacted Files Coverage Δ
databricks/koalas/frame.py 96.51% <92.85%> (+3.2%) ⬆️
databricks/koalas/series.py 96.39% <0%> (+0.13%) ⬆️
databricks/koalas/groupby.py 91.43% <0%> (+0.21%) ⬆️
databricks/koalas/testing/utils.py 78.51% <0%> (+0.74%) ⬆️
databricks/koalas/plot.py 94.28% <0%> (+0.95%) ⬆️
databricks/koalas/namespace.py 87.87% <0%> (+1.51%) ⬆️
databricks/conftest.py 96.22% <0%> (+7.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 600d48d...59d50c3. Read the comment docs.

@@ -18,6 +18,7 @@ pypandoc
ipython

# Linter
black==19.10b0; python_version >= "3.6"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this file is being shared with conda of the CI too, which has a slightly different syntax comparing to the regular PIP.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep... I was hopeful conda/conda-build#1984 supported it, but I think it's for something different. :( I'll add it differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I was trying to figure out yesterday where the other library requiring Python (Conda) 3.6+ is and couldn't find it; figured I'd support this similarly.

https://github.com/databricks/koalas/blob/master/.travis.yml#L69

@itholic
Copy link
Contributor

itholic commented Feb 17, 2020

maybe i think we have same discussed about using Black before.

can you refer to this PR and this comment ?

@deepyaman
Copy link
Contributor Author

maybe i think we have same discussed about using Black before.

can you refer to this PR and this comment ?

I saw previous discussions (#755 and #756), but I think I missed that part of the discussion on #761. I would be feel pretty against using Black just on diffs--rather than a consistent code style, you end up with an even worse mix between old and now.

If there are certain formatting rules would like to tweak, Black supports that, as long as those options are applied consistently throughout.

@deepyaman deepyaman changed the title [WIP] Standardize code style using Black Standardize code style using Black Feb 17, 2020
@HyukjinKwon
Copy link
Member

I would be feel pretty against using Black just on diffs--rather than a consistent code style, you end up with an even worse mix between old and now.

The problem is that, it overwrites the commit logs and difficult to track the changes. Also, I would like to keep it as an option, not a hard requirement because some people prefers a different style.

After all, we still run multiple linters so it's fine as long as it follows. PEP 8

@@ -0,0 +1,6 @@
repos:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I was thinking about using .git/hooks/pre-commit right away rather then using a dependency for now.

@HyukjinKwon
Copy link
Member

Okay, let me take a look and fix it if it's really worthwhile. I will port some changes here and make the commit co-authored.

@HyukjinKwon
Copy link
Member

Let me make #1301 co-authored. thanks @deepyaman!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add precommit in workflow to standardize code style
4 participants