kevinjqliu commented on code in PR #1432:
URL: https://github.com/apache/iceberg-python/pull/1432#discussion_r1887143738
##########
pyiceberg/types.py:
##########
@@ -328,8 +328,8 @@ def __init__(
data["type"] = data["type"] if "type" in data else field_type
data["required"] = required
data["doc"] = doc
Review Comment:
I went down a rabbit hole, here's what I learned.
1. Using `__init__` with a pydantic model is kind of an anti-pattern; the
pydantic model typically handles all the initialization/validation.
2. `__init__` here is to provide the ability to initialize `NestedField`
with positional args, i.e. `NestedField(1, "blah")`
For backward compatibility, we can't change `NestedField` to not take
positional args.
But we can make this `__init__` (and other `__init__`s) more resilient to
bugs with something like
```
def __init__(self, *args, **kwargs):
# implements `__init__` to support initialization with positional
arguments
if args:
field_names = list(self.model_fields.keys()) # Gets all field
names defined on the model
if len(args) > len(field_names):
raise TypeError(f"Too many positional arguments. Expected at
most {len(field_names)}")
kwargs.update(dict(zip(field_names, args))) # Maps positional
args to field names
# Let Pydantic handle aliases and validation
super().__init__(**kwargs)
```
This doesn't check for when using both positional and keyword args, i.e.
`NestedField(1, field_id=10)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
