-
Notifications
You must be signed in to change notification settings - Fork 327
chore(enhancement): rename python package to apache-polaris #2812
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
base: main
Are you sure you want to change the base?
chore(enhancement): rename python package to apache-polaris #2812
Conversation
0d34b23 to
395cb27
Compare
4e6bbc6 to
e797543
Compare
|
@MonkeyCanCode @HonahX Hello colleagues. Please review. Thx 🙏 |
|
Hello, sorry for the delay. I will check this weekend.
|
| `apache_polaris.sdk.catalog.models.LoadPolicyResponse`. | ||
| However, the generator fails to add the necessary `import` statements | ||
| to the `polaris/catalog/models/__init__.py` file. This means that even |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
polaris/catalog/models/__init__.py is now outdated with this PR. Change to apache_polaris/sdk/catalog/models/__init__.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@binarycat0 mind update here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, good catch! I need to double-check all places properly 👍
| `apache_polaris.sdk.catalog.models.LoadPolicyResponse`. | ||
| However, the generator fails to add the necessary `import` statements | ||
| to the `polaris/catalog/models/__init__.py` file. This means that even |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path polaris/catalog/models/__init__.py is outdated. Change to apache_polaris/sdk/catalog/models/__init__.py
@binarycat0 nice work and thanks for the coordination on the dev ML. |
- reorganize python package catalogs - union all submodules (cli, polaris) under apache_polaris module - put polaris generated context into apache_polaris/sdk - make apache_polaris as a main module
15c0342 to
d97136e
Compare
|
Hello @MonkeyCanCode 👋, thank you for the review. I fixed the mentioned NIT comments, thanks. Also, the mentioned string splitters by () is caused by automatically formatting or by In my experience an applying "isort + mypy + black" linting was by default, I would like suggest following the same standard as many python projects.
|
Thanks for the quick fix. One of the minor comment was not fixed. Other than that, this looks good to me. @eric-maynard @HonahX , do u mind also take a look when u have a chance? |
|
@MonkeyCanCode Thank you again. I double-checked all places, now it should be good. |
cli,polaris) underapache_polarismodulepolarisgenerated context intoapache_polaris/sdkapache_polarisas a main moduleFixes: #2699
What changes were proposed in this pull request?
Package name
As discussed in the mailing list thread, the majority agreed that a more suitable and compliant name would be
apache-polarisaccording to the Apache Distribution guideCatalogs structure
It would be beneficial to modify the directory structure. The current setup can lead to conflicts after package installation because it doesn't create a parent directory to group all apache-polaris related modules together. Instead, it directly places the cli and polaris directories into the root
.../site-packagesof the active environment.In the structure I am proposing, all modules related to the project will be placed in the
apache_polarismodule and will be available for use in code through import apache_polaris.sdk.catalog, apache_polaris.sdk.management and import apache_polaris.cli respectively.Why are the changes needed?
Initial Issue: #2419
Follow up Issue: #2699
Does this PR introduce any user-facing change?
How was this patch tested?
CHANGELOG.md