datastrato / gravitino

World's most powerful data catalog service with providing a high-performance, geo-distributed and federated metadata lake.
https://datastrato.ai/docs/
Apache License 2.0
347 stars 150 forks source link

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

Closed noidname01 closed 1 week ago

noidname01 commented 2 weeks ago

What changes were proposed in this pull request?

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

jerryshao commented 1 week ago

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

noidname01 commented 1 week ago

@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 commented 1 week ago

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

noidname01 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

@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.

jerryshao commented 1 week ago

@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 commented 1 week ago

@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 commented 1 week ago

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

jerryshao commented 1 week ago

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