delta-io / delta

An open-source storage framework that enables building a Lakehouse architecture with compute engines including Spark, PrestoDB, Flink, Trino, and Hive and APIs
https://delta.io
Apache License 2.0
6.98k stars 1.6k forks source link

Add python linting to circleci build #214

Closed SpaceRangerWes closed 4 years ago

SpaceRangerWes commented 4 years ago

Shamelessly stolen from https://github.com/apache/spark/blob/master/dev/lint-python

rahulsmahadev commented 4 years ago

Hey do you need any help on this ? it's still failing the build

SpaceRangerWes commented 4 years ago

Possibly. I just haven’t had the time to address it with my current work deadlines taking priority.

Sent from ProtonMail Mobile

On Wed, Oct 30, 2019 at 11:28 AM, Rahul Shivu Mahadev notifications@github.com wrote:

Hey do you need any help on this ? it's still failing the build

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

SpaceRangerWes commented 4 years ago

@rahulsmahadev I've fixed the pycodestyle errors. We are now getting more valid flake8 errors coming out of that step.

rahulsmahadev commented 4 years ago

@rahulsmahadev I've fixed the pycodestyle errors. We are now getting more valid flake8 errors coming out of that step.

@SpaceRangerWes Can we fix those flake8 errors in this PR if they are easy to fix.

rahulsmahadev commented 4 years ago

@SpaceRangerWes can you fix the flake8 issues it's 3-4 lines in docs/generate_api_docs.py. Basically the print statements need to be of the type print() instead of print.

EDIT: Attempting to fix the flake 8 errors.

rahulsmahadev commented 4 years ago

@SpaceRangerWes It is building now, but we may need to add the license from Apache Spark to the files "borrowed" from it.

SpaceRangerWes commented 4 years ago

@SpaceRangerWes It is building now, but we may need to add the license from Apache Spark to the files "borrowed" from it.

@rahulsmahadev I've updated the license headers in the two new files with those from the Spark project. Thanks for all the help. At this point this PR should be your credit and not mine.

rahulsmahadev commented 4 years ago

@tdas Can you have a look at this PR as well ?

tdas commented 4 years ago

This looks good to me. Will merge soon.

tdas commented 4 years ago

Actually, just one comment. Can you move the code to run the python linting into the python/run-tests.py, so that all the python related stuff stay contained in python subdir?

SpaceRangerWes commented 4 years ago

Actually, just one comment. Can you move the code to run the python linting into the python/run-tests.py, so that all the python related stuff stay contained in python subdir?

@tdas just to be clear, you want all the code in the file? Or do you want the new files in the python subdir?

tdas commented 4 years ago

Aah right. I was ambiguous. I was referring to the code snippet def run_python_style_checks(root_dir): that you added to ROOT_DIR/run-tests.py. My suggestion was to put it in the ROOT_DIR/python/run-tests.py so that all the python-related tests can be run together by running python/run-tests.py.

tdas commented 4 years ago

@SpaceRangerWes python style tests are failing :) other than that this is good to go.

SpaceRangerWes commented 4 years ago

@tdas yeah... I introduced a PEP format error in the run-tests.py :(

tdas commented 4 years ago

Hey, that just means that this style-check works! :)

SpaceRangerWes commented 4 years ago

@tdas all good now.

tdas commented 4 years ago

@SpaceRangerWes Can you merge with master branch and try it again. I had trouble merging this PR because it hung while running the tests.

[tdas @ ~/Projects/Databricks/delta-td] [(HEAD detached from oss/pr/214) ?] dev/lint-python
downloading pycodestyle from https://raw.githubusercontent.com/PyCQA/pycodestyle/2.4.0/pycodestyle.py...
starting pycodestyle test...

I think I narrowed the issue to the ./examples/tutorials file names, which have spaces in them (my fault) and may be causing issues. Your PR branch does not seem to have those files at all because you have not merged with the master in a long time.

tdas commented 4 years ago

@SpaceRangerWes @rahulsmahadev ignore this reopening of the closed PR. I have been testing our internal scripts to manage the PRs and I wanted to test it with a test PRs with multiple authors. Hence I opened this. In fact, I opened #360 ignore that too.