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? |
|
@cpcloud should I invest more time in this or do you think this whole idea is not the right direction? |
|
I'm not opposed to this but I think the behavior should be different for attribute versus getitem syntax. It all comes down to what we believe the user's intent is, to the best of our knowledge. For attribute misspellings, I think we should leave them as plain old For |
|
That sounds like good behavior to me to return an AttributeError for There is one bit of nuance on attribute access though. If I do In pseudocode: # Raised on t.bogus
# Includes helpful suggestion "did you mean <best match among all columns AND attributes/methods>"
class TableAttributeError(AttributeError, IbisError): ...
# Raised on my_struct.bogus
# Includes helpful suggestion "did you mean <best match among all fields AND attributes/methods>"
# Maybe should get consolidated with TableAttributeError
class StructAttributeError(AttributeError, IbisError): ...
# Raised on table_or_struct["bogus", "bogus2"] and table_or_struct.select(_.bogus, _.bogus2) and my_struct["bogus"]
# Includes helpful suggestion "did you mean <best match among ONLY columns/fields>"
# Note that this contains ALL not found fields.
# I think that would be simpler to just have one error, and not one for single FieldNotFound.
# Should this inherit from AttributeError and/or KeyError?
# I think AttributeError since 1. these are statically known and 2. then a user can `catch AttributeError`
# and have all of these errors caught.
class FieldsNotFoundError(AttributeError, IbisError): ...What do you think of this spec? |
So I understand your goals, is your reasoning here so that 1. our maintenance is easier or 2. so we don't accidentally do the wrong thing for the user? |
I have been getting sick of typing
some_table_or_struct.field_that_doesnt_exist_or_has_a_small_typoand then getting a useless error message. It also is annoying to dosome_table.select(doesnt_exist, also_doesnt_exist), and you only get an error for the very first one. It would be better if you got back info on ALL the values that failed to get resolved.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?