[#3206]improvement(client-python): Add Black for client-python

[noidname01]

What changes were proposed in this pull request?

• Add Black as code formatter, it enforce the coding style with
  1. trailing commas and whitespaces
  2. unified quotation marks(")
  3. new lines
  4. max line length
  5. indents
• Aligned with Pylint Rules
• Add Black into Gradle and form a code formatting pipeline (pipInstall -> Black -> Pylint), this pipeline will run implicitly in build and test gradle tasks.

Note that Black still can't format long entire string exceeding max line length without enabling unstable features, please handle long strings with caution and make Pylint to ignore them if they are really necessary.

Why are the changes needed?

Fix: #3206, #3203

Does this PR introduce any user-facing change?

No

How was this patch tested?

No

[xunliu]

@noidname01 Thank you for you improving code style of Gravitino Python client.
Now Python code become to neat and elegant.

[jerryshao]

@noidname01 what is the relationship between pylint and black? How do we trigger this in gradle?

[noidname01]

@jerryshao Currently the black (formatter) will run before pylint(linter), this is enforced by the mustRunAfter function in Gradle.

This pipeline was manually defined to run before test and build tasks by using dependsOn(pipInstall, black, pylint).
I've seen your reply and I will modify the Gradle script to trigger this pipeline automatically before doing any task in client-python project.

[jerryshao]

black is integrated into spotless, can we use spotless one?

[noidname01]

I had just survey the spotless Python solution and I found it need to manually install black to local environment, not a virtual environment, the automatic way is still a TODO.
IMO, I think it's better to leave all python dependencies in virtual environments.

[jerryshao]

I had just survey the spotless Python solution and I found it need to manually install black to local environment, not a virtual environment, the automatic way is still a TODO. IMO, I think it's better to leave all python dependencies in virtual environments.

I see, thanks for the explain.

[noidname01]

@jerryshao Currently the black (formatter) will run before pylint(linter), this is enforced by the mustRunAfter function in Gradle.

This pipeline was manually defined to run before test and build tasks by using dependsOn(pipInstall, black, pylint). I've seen your reply and I will modify the Gradle script to trigger this pipeline automatically before doing any task in client-python project.

I've add finalizedBy in envSetup task(a task brought by VenvTask) to force every task run pip install and code formatting pipeline before their own tasks.
But I found client-python was not even included in compileDistribution task, so I think it's better to run python formatting pipeline when it's necessary.

[xunliu]

Which mean are these codes?

[xunliu]

hi @noidname01 I didn't found envSetup task in you PR.

[noidname01]

The envSetup task is from VenvTask, this task is respnsible for python virtual environment setup, so I think it's a great entry to bind our pipeline before running all the other tasks.
The source code is here.

[jerryshao]

@jerryshao Currently the black (formatter) will run before pylint(linter), this is enforced by the mustRunAfter function in Gradle.
This pipeline was manually defined to run before test and build tasks by using dependsOn(pipInstall, black, pylint). I've seen your reply and I will modify the Gradle script to trigger this pipeline automatically before doing any task in client-python project.

I've add finalizedBy in envSetup task(a task brought by VenvTask) to force every task run pip install and code formatting pipeline before their own tasks. But I found client-python was not even included in compileDistribution task, so I think it's better to run python formatting pipeline when it's necessary.

@noidname01 sorry for late response. It is not needed for client-python to be included in compileDistribution task. compileDistribution task is used to package the Gravitino server, not the client stuffs. For clients like java, python, they will only publish to the central repository like maven, pypi, they will not ship with server binary package.

[noidname01]

@jerryshao Oh I think I gave you wrong idea because of my expression, what I mean is we only need to run python code formatting pipeline when needed, not including the compileDistribution task which is for gravitino server.

[jerryshao]

II would suggest to change to 100 to align with our java code. WDYT @noidname01 @xunliu , what is the typical length of python code?

[noidname01]

This is because Black seems not to format the length of docstrings but Pylint will complain about it.
There are many long docstrings in current project, I can try to refine them, But I think this PR has done too many modifications, maybe finish in follow-up PRs ?

BTW, the max length is defined as 79 characters each line in PEP8(Python Standard Library Code Style), and the default setting of Black is 88.

[jerryshao]

Yeah, you can do it in a separate PR. the default length seems too short, if so I would suggest to align with Java code to use 100.

[jerryshao]

I tried it locally, generally LGTM, just have a minor comment. @xunliu would you please take a deep look when you have time?

[xunliu]

@noidname01 Thank you for your contributions.
LGTM

[jerryshao]

@noidname01 would you please create a PR to cherry-pick this merged commit to branch-0.5.