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

feat(docs): example of multiple catalogs defined in .pyiceberg.yaml #194

Merged
merged 3 commits into from
Dec 9, 2023

Conversation

jayceslesar
Copy link
Contributor

Spawned from #156

Copy link
Contributor

@HonahX HonahX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @jayceslesar. Just a quick, non-blocking comment from my side

uri: https://rest-server:8181/
warehouse: my-warehouse
```
and loaded in python by calling `load_catalog`. See below for an example.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the example primarily illustrates how to load a catalog that is not defined in .pyiceberg.yaml. For catalogs that are defined in the yaml file, we might say

and loaded in python by calling `load_catalog(name="hive")` and `load_catalog(name="rest")`

Does this sound right to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that is more explicit -- will add

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jayceslesar for fixing this! This was super obvious for me, but that's mostly because I wrote the code initially, thanks for adding this example 🙌

Thanks @HonahX for the prompt review!

@Fokko
Copy link
Contributor

Fokko commented Dec 9, 2023

@jayceslesar The Markdown linter has an improvement, can you run make lint?

@jayceslesar
Copy link
Contributor Author

@Fokko my org has a manual job that can be run as part of a PR/MR that can run the lint and commit the changes? Is it worth looking at adding something similar? Or stick with the pre-commit? (which I just need to get used to haha)

@Fokko
Copy link
Contributor

Fokko commented Dec 9, 2023

my org has a manual job that can be run as part of a PR/MR that can run the lint and commit the changes? Is it worth looking at adding something similar?

Ha, I'm aware of those systems, and I like them! I haven't looked into that much, but this would require committing on someone's behalf?

@Fokko Fokko merged commit 043aba5 into apache:main Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants