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

Library public api isolation and import decoupling #499

Closed
ndrluis opened this issue Mar 5, 2024 · 9 comments
Closed

Library public api isolation and import decoupling #499

ndrluis opened this issue Mar 5, 2024 · 9 comments
Assignees
Labels

Comments

@ndrluis
Copy link
Collaborator

ndrluis commented Mar 5, 2024

Feature Request / Improvement

Hello, when I was trying to solve the #497 issue, I noticed that we are exposing the private API and we have some imports through modules.

For example, in glue.py, we import Identifier and Properties from catalog instead of importing from typedef. Another case is in test_writes.py, where we import Table from catalog instead of from table.

@HonahX
Copy link
Contributor

HonahX commented Mar 6, 2024

@ndrluis Thanks for reporting this! I think we should fix these to import from the right module. Do you want to work on this?

@ndrluis
Copy link
Collaborator Author

ndrluis commented Mar 6, 2024

@HonahX Yes!

@ndrluis
Copy link
Collaborator Author

ndrluis commented Mar 7, 2024

The #505 decouple imports from

  • pyiceberg.catalog
  • pyiceberg.table
  • pyiceberg.expressions

@kevinjqliu
Copy link
Contributor

Thanks for the PR! Do you know if there are ways to do this systemically, with linters / rules? I foresee this to be an issue again as we continue to do more development.

@ndrluis
Copy link
Collaborator Author

ndrluis commented Mar 8, 2024

@kevinjqliu Agreed, I haven't found anything that addresses this issue or provides a warning when it occurs. It seems like something the community has accepted, but I believe it's a problem. I will open an issue on Ruff to ask about this and understand how we can write a rule.

@ndrluis
Copy link
Collaborator Author

ndrluis commented Mar 8, 2024

Issue to follow up astral-sh/ruff#10300

@ndrluis
Copy link
Collaborator Author

ndrluis commented Mar 12, 2024

We have some news about the issue: mypy has implemented a rule that could protect us. You can find more details here: https://mypy.readthedocs.io/en/stable/config_file.html#confval-implicit_reexport. Furthermore, there is an ongoing discussion about implementing this rule in ruff.

Copy link

github-actions bot commented Sep 9, 2024

This issue has been automatically marked as stale because it has been open for 180 days with no activity. It will be closed in next 14 days if no further activity occurs. To permanently prevent this issue from being considered stale, add the label 'not-stale', but commenting on the issue is preferred when possible.

@github-actions github-actions bot added the stale label Sep 9, 2024
@ndrluis
Copy link
Collaborator Author

ndrluis commented Sep 9, 2024

I'll close this issue because we added the mypy linter, which solves our problem with coupling, and we have #1099 to solve the public API definition.

@ndrluis ndrluis closed this as completed Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants