chore: simplify multi-nested try blocks #2114
Conversation
Instead of have a multi-nested series of try blocks. Convert it to a more readable series of `if` statements.
Codecov Report
@@ Coverage Diff @@
## main #2114 +/- ##
==========================================
- Coverage 95.34% 95.34% -0.01%
==========================================
Files 78 78
Lines 5090 5087 -3
==========================================
- Hits 4853 4850 -3
Misses 237 237
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -101,47 +101,43 @@ def __setstate__(self, state: Dict[str, Any]) -> None: | |||
| self.__dict__["_module"] = importlib.import_module(module_name) | |||
|
|
|||
| def __getattr__(self, name: str) -> Any: | |||
There was a problem hiding this comment.
Nice to see things get flattened! 😁
I'm a bit torn on the EAFP/LBYL topic here, using try/except on the re-raise would allow us to keep the original raise-from, though not sure how important that is. I would maybe also consider a compromise like this:
def __getattr__(self, name: str) -> Any:
try:
return self.__dict__["_updated_attrs"][name]
except KeyError:
pass
try:
value = self.__dict__["_attrs"][name]
# If the value is a list, we copy it in the _updated_attrs dict
# because we are not able to detect changes made on the object
# (append, insert, pop, ...). Without forcing the attr
# creation __setattr__ is never called, the list never ends up
# in the _updated_attrs dict, and the update() and save()
# method never push the new data to the server.
# See https://github.com/python-gitlab/python-gitlab/issues/306
#
# note: _parent_attrs will only store simple values (int) so we
# don't make this check in the next except block.
if isinstance(value, list):
self.__dict__["_updated_attrs"][name] = value[:]
return self.__dict__["_updated_attrs"][name]
return value
except KeyError:
pass
try:
return self.__dict__["_parent_attrs"][name]
except KeyError as exc:
message = (
f"{type(self).__name__!r} object has no attribute {name!r}"
)
if self._created_from_list:
message = (
f"{message}\n\n"
+ textwrap.fill(
f"{self.__class__!r} was created via a list() call and "
f"only a subset of the data may be present. To ensure "
f"all data is present get the object using a "
f"get(object.id) call. For more details, see:"
)
+ f"\n\n{_URL_ATTRIBUTE_ERROR}"
)
raise AttributeError(message) from excNot sure. Thoughts?
There was a problem hiding this comment.
Thanks for the review!
Nice to see things get flattened! 😁
I'm a bit torn on the EAFP/LBYL topic here, using try/except on the re-raise would allow us to keep the original raise-from, though not sure how important that is. I would maybe also consider a compromise like this:
Not sure. Thoughts?
I don't see any value in keeping the original exception as that is an exception of our creation because the code is indexing into an array and failing. When attribute access fails in normal objects they don't raise a KeyError they raise an AttributeError.
>>> x = 2
>>> x.hello
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'int' object has no attribute 'hello'
To me EAFP makes sense for like opening a file. Trying to figure out all the ways something can fail to prevent opening a file, it is way better to just try/except.
But in this case we just want to know is the item in one of the three dictionaries _updated_attrs, _attrs, or _parent_attrs. The only way it seems it can go wrong is that the key is not found in the dictionary.
Which one do you find easier to read?
There was a problem hiding this comment.
Agreed the exception doesn't really matter. I think I just usually don't see these double dict lookups with if conditionals used so much in python, so try/except would be the go-to for me.
Instead of have a multi-nested series of try blocks. Convert it to a
more readable series of
ifstatements.