feat(api): add FieldNotFoundError #10412
Conversation
NickCrews
force-pushed
the
field-not-found-error
branch
from
2fae0d4 to
8be679f
Compare
| values = [] | ||
| errs = [] | ||
| # bind positional arguments | ||
| for arg in args: |
There was a problem hiding this comment.
This is somewhat orthogonal to the FieldNotFoundError stuff. But if I have multiple bogus columns, eg table.select("bogus", "also_bogus") I only see the first error. I want to see them all.
| if len(bindings) != 1: | ||
| raise com.IbisInputError( | ||
| "Keyword arguments cannot produce more than one value" | ||
| ) | ||
| (value,) = bindings | ||
| values.append(value.name(key)) | ||
| if errs: | ||
| raise com.IbisError( |
There was a problem hiding this comment.
not sure if IbisError is the best type for this.
| @@ -739,8 +757,9 @@ def __getattr__(self, key: str) -> ir.Column: | |||
| """ | |||
| try: | |||
| return ops.Field(self, key).to_expr() | |||
| except com.IbisTypeError: | |||
| pass | |||
| except com.FieldNotFoundError as e: | |||
There was a problem hiding this comment.
There is a slight difference in ux here that I'd love to unify
- if I do
Table.totally_bogus, then I getAttributeError: 'Table' object has no attribute 'bogus' - if I do
Table["totally_bogus"], then I getFieldNotFoundError: 'bogus' not found in Table object. Possible options: {'x'}
I'm not sure which is better. If we say Possible options:... then that only includes the field names, and misses all the Table methods. But, all methods are 1. in the docs and 2. should have tab completion in many cases, so I bet typos are a lot more likely on column typos than method typos. So I think the FieldNotFoundError with the suggestion might be better.
There was a problem hiding this comment.
Also a difference between Tables and Structs. I'd love for them to have the same behavior too.
| @@ -205,7 +205,7 @@ def __getitem__(self, name: str) -> ir.Value: | |||
| KeyError: 'foo_bar' | |||
| """ | |||
| if name not in self.names: | |||
| raise KeyError(name) | |||
| raise FieldNotFoundError(self, name, self.names) | |||
There was a problem hiding this comment.
I think it's OK to be breaking here and not raise a KeyError anymore?
There was a problem hiding this comment.
KeyError seems semantically a little wrong. I think KeyError should be for collections with a dynamic set of keys, such as a vanilla python dict. But structs have a static set of keys, so FieldNotFoundError, as a subclass of AttributeError, seems better to me.
I have been getting sick of typing some_table_or_struct.field_that_doesnt_exist_or_has_a_small_typo and then getting a useless error message. This PR makes that UX much better. Still need to add tests, but I wanted to get this up here for some initial thoughts before I invested more time. Is this something we want to pursue?
NickCrews
force-pushed
the
field-not-found-error
branch
from
8be679f to
c5f58d5
Compare
|
@cpcloud does this look like an idea worth considering? Any behavior requirements that you think I would need to make an implementation satisfy? |
I have been getting sick of typing some_table_or_struct.field_that_doesnt_exist_or_has_a_small_typo and then getting a useless error message. This PR makes that UX much better.
Still need to add tests, but I wanted to get this up here for some initial thoughts before I invested more time. Is this something we want to pursue?