1191:Added docstrings to the pyiceberg/table/inspect.py file by gayatrikate04 · Pull Request #1533 · apache/iceberg-python

gayatrikate04 · 2025-01-17T14:59:54Z

Added detailed docstrings to the pyiceberg/table/inspect.py file to improve documentation and code clarity. The updates enhance readability and help developers understand the functionality of the InspectTable class and its methods.

Fokko · 2025-01-17T15:32:45Z

pyiceberg/table/inspect.py

Let's avoid changing the LICENSE, the linebreak is now also a bit awkward

kevinjqliu

added a few comments here, thanks for the PR

pyiceberg/table/inspect.py

kevinjqliu · 2025-01-17T17:38:38Z

pyiceberg/table/inspect.py

this is cut off

kevinjqliu · 2025-01-17T17:40:34Z

pyiceberg/table/inspect.py

If not provided, all partitions are included.

I dont think this is right, the snapshot_id for all of these functions are used to time travel to a specific snapshot. otherwise the current snapshot will be used

kevinjqliu · 2025-01-17T17:41:06Z

pyiceberg/table/inspect.py

imports should be on top of the file

kevinjqliu · 2025-01-17T17:41:24Z

pyiceberg/table/inspect.py

Suggested change

class IcebergTableUtils:

i dont think we need a new class here

gayatrikate04 · 2025-01-17T17:57:06Z

"Thank you for the detailed feedback! I'll make the following updates:
Fix the cutoff docstring to provide a complete explanation.
Update the description of snapshot_id to accurately reflect its role in time traveling to specific snapshots.
Move the imports to the top of the file to align with standard practices.
Remove the unnecessary class or refactor it if required.
I'll address these issues and push the changes .

gayatrikate04 · 2025-01-21T17:40:35Z

I have pushed the latest changes, including refinements to the docstrings and updates to related files. Please review the updates.

kevinjqliu

Thanks for the contribution! I've added some comments, i think theres a linter error, could you run make lint locally?

kevinjqliu · 2025-01-22T15:34:37Z

.python-version

this is part of pyenv's local config, should not be checked into the repo

Got it! I will remove .python-version from the PR.

kevinjqliu · 2025-01-22T15:35:06Z

mkdocs/docs/SUMMARY.md

why was this changed? i dont think this is necessary

I will remove the change from the SUMMARY.md file if it's not necessary. Thanks for pointing it out!

Please revert the change so we can get this in :)

kevinjqliu · 2025-01-22T15:35:59Z

poetry.lock

can you rebase this PR against main to get the latest change from #1538?

Thank you for the suggestion! I’m not very familiar with rebasing yet, but I’m eager to learn. Could you guide me on how to properly rebase this PR against the main branch, or would merging the latest changes from #1538 be a better approach in this case? I want to ensure I’m following the best practices.

kevinjqliu · 2025-01-22T15:36:37Z

mkdocs/mkdocs.yml

same for this, what is this change for?

Thank you for pointing this out! I made this change while addressing errors in inspect.py. However, I’ll review it again to ensure it’s necessary and aligned with the overall structure. Please let me know if you have additional context or suggestions.

kevinjqliu · 2025-01-22T15:40:27Z

pyiceberg/table/inspect.py

 from pyiceberg.utils.singleton import _convert_to_hashable_type

 if TYPE_CHECKING:
-    import pyarrow as pa


i think we still need this here

kevinjqliu · 2025-01-22T15:43:43Z

pyiceberg/table/inspect.py

            raise ValueError("Cannot get a snapshot as the table does not have any.")

    def snapshots(self) -> "pa.Table":
-        import pyarrow as pa


i think we actually need this here, in case someone imports this function directly

from pyiceberg.table.inspect import snapshots

I understand. I will keep the import for pyarrow as suggested, in case the function is used directly.

kevinjqliu · 2025-01-22T15:46:10Z

pyiceberg/table/inspect.py

i like this description of how snapshot_id is used, can we apply this for all similar functions
perhaps something more generic

Suggested change

Args:

snapshot_id (Optional[int]): The ID of the snapshot to retrieve entries for.

If None, entries for the current snapshot are returned.

Args:

snapshot_id (Optional[int]): The ID of the snapshot to retrieve. If None, the current snapshot is used.

Thank you for the feedback! I’m glad you liked the description. I’ll review other similar functions and update their docstrings to use a more generic and consistent phrasing as suggested. Let me know if there are any additional improvements you'd like to see

Fokko

Thanks for working on this @gayatrikate04 The docstrings are great, but could you follow up on the review of @kevinjqliu? Would be great to get this in 👍

Fokko · 2025-02-03T11:23:23Z

mkdocs/docs/SUMMARY.md

Please revert the change so we can get this in :)

Fokko · 2025-02-03T11:23:29Z

.python-version

Can you remove this file?

gayatrikate04 · 2025-02-03T17:06:15Z

I have removed the .python-version file and reverted the changes in SUMMARY.md.

kevinjqliu · 2025-02-03T21:47:21Z

can you also revert the changes to mkdocs/mkdocs.yml, poetry.lock, and pyproject.toml?

since this is about adding documentations, i think the only change should be in pyiceberg/table/inspect.py

….toml

gayatrikate04 · 2025-02-04T14:40:54Z

I have reverted the changes to mkdocs/mkdocs.yml, poetry.lock, and pyproject.toml

gayatrikate04 · 2025-02-18T16:31:01Z

I've resolved the merge conflicts in poetry.lock and pyproject.toml and pushed the changes.

gayatrikate04 · 2025-02-26T16:52:44Z

resolved conflicts

Gayatri Kate added 2 commits January 17, 2025 19:01

Added docstrings to the inspect.py file

14a607a

Added docstrings to pyiceberg/table/inspect.py for better documentation

f71235c

Fokko reviewed Jan 17, 2025

View reviewed changes

kevinjqliu reviewed Jan 17, 2025

View reviewed changes

Refined docstrings in inspect.py and updated related files

a8f41d1

kevinjqliu reviewed Jan 22, 2025

View reviewed changes

gayatrikate04 mentioned this pull request Jan 29, 2025

1191:Added Docstring and Fixed linter error #1590

Closed

Gayatri Kate added 2 commits January 30, 2025 22:22

Fixed linter error

6c180a9

Removed mkdocs dependency and updated lock file

1dfd302

Fokko reviewed Feb 3, 2025

View reviewed changes

Remove .python-version file and revert changes in SUMMARY.md

5fc6489

Reverted the changes to mkdocs/mkdocs.yml, poetry.lock, and pyproject…

85a21ac

….toml

Merge branch 'main' into add-docstrings-inspect/1191

ee507fb

gayatrikate04 added 2 commits February 22, 2025 18:09

Merge branch 'main' into add-docstrings-inspect/1191

6f49701

Merge branch 'main' into add-docstrings-inspect/1191

c37c8e4

-         Args:
-            snapshot_id (Optional[int]): The ID of the snapshot to retrieve entries for.
-              If None, entries for the current snapshot are returned.
+         Args:
+            snapshot_id (Optional[int]): The ID of the snapshot to retrieve. If None, the current snapshot is used.

Conversation

gayatrikate04 commented Jan 17, 2025 • edited by kevinjqliu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gayatrikate04 commented Jan 17, 2025

Uh oh!

gayatrikate04 commented Jan 21, 2025

Uh oh!

kevinjqliu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fokko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gayatrikate04 commented Feb 3, 2025

Uh oh!

kevinjqliu commented Feb 3, 2025

Uh oh!

gayatrikate04 commented Feb 4, 2025

Uh oh!

gayatrikate04 commented Feb 18, 2025

Uh oh!

gayatrikate04 commented Feb 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

gayatrikate04 commented Jan 17, 2025 •

edited by kevinjqliu

Loading

kevinjqliu left a comment •

edited

Loading