docs: Add docstrings for Identifier, Properties, RecursiveDict

[rodrigc]

This allows mkdocstrings to generate hyperlinks
to these types in the documentation.

Fixes: #1529

[kevinjqliu]

Thanks for working on this.
Looks like there's a linter issue in CI, you can fix it by running make lint locally.

Also, it would be helpful to see the rendered doc page for these fields along with showing that it is hyperlinked in create_table_if_not_exists

[rodrigc]

The issue with the linter is that it does not allow docstrings to appear after the variable.

For this code:

Identifier = Tuple[str, ...]
"""A tuple of strings representing a table identifier.

Each string in the tuple represents a part of the table's unique path. For example,
a table in a namespace might be identified as:

    ("namespace", "table_name")

Examples:
    >>> identifier: Identifier = ("namespace", "table_name")
"""

Properties = Dict[str, Any]
"""A dictionary type for properties in PyIceberg."""


RecursiveDict = Dict[str, Union[str, "RecursiveDict"]]
"""A recursive dictionary type for nested structures in PyIceberg."""

The linter fails with:

- hook id: check-docstring-first
- exit code: 1

pyiceberg/typedef.py:77: Module docstring appears after code (code seen on line 17).

If I change to:

"""A tuple of strings representing a table identifier.

Each string in the tuple represents a part of the table's unique path. For example,
a table in a namespace might be identified as:

    ("namespace", "table_name")

Examples:
    >>> identifier: Identifier = ("namespace", "table_name")
"""
Identifier = Tuple[str, ...]

"""A dictionary type for properties in PyIceberg."""
Properties = Dict[str, Any]


"""A recursive dictionary type for nested structures in PyIceberg."""
RecursiveDict = Dict[str, Union[str, "RecursiveDict"]]

the linter fails with:

check docstring is first.................................................Failed
- hook id: check-docstring-first
- exit code: 1

pyiceberg/typedef.py:76: Module docstring appears after code (code seen on line 17).

Any ideas as to the best way forward?

[pawamoy]

Docstrings must definitely be written below attribute assignments for mkdocstrings to pick them up. I'd recommend disabling this lint rule as it apparently doesn't support code with several docstrings at the module-level (which is something Python itself happily deals with, using only the first docstring as the module's __doc__ attribute).

[rodrigc]

I see what you are saying. It looks like this issue has come up before:
pre-commit/pre-commit-hooks#159

This project chose to remove the check-docstring-first pre-commit hook:
tiacsys/verylittlewire@07b2cd2

Do you suggest that pyiceberg do the same?
Should that sort of change be done in a separate PR, or can it be combined with this one?

[rodrigc]

@pawamoy I submitted a separate PR: #1531 to remove check-docstring-first

[pawamoy]

@rodrigc do note that I'm just a passer-by! I have no authority here 😄 I'm the maintainer of mkdocstrings and found this issue while stalking a bit, just wanted to help 🙂 I understand it's not great to just drop the lint rule (because it can be helpful), but I'm not sure if there's a better rule out there 🤷 I don't use pre-commit (the tool) myself.

[kevinjqliu]

Looks like #1531 is merged, could you rebase this PR?

[rodrigc]

I rebased this PR. Do you have any further suggestions for improving the docstrings for Properties and RecursiveDict?

[kevinjqliu]

LGTM!

Properties and RecursiveDict are pretty generic types so the current docstring looks good to me

[kevinjqliu]

Could you add some screenshots to the PR description showing the hyperlink?

[rodrigc]

[kevinjqliu]

Amazing, thank you!