Skip to content

chore: simplify multi-nested try blocks #2114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 4, 2022
Merged

chore: simplify multi-nested try blocks #2114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 36 additions & 40 deletions gitlab/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 exc

Not sure. Thoughts?

Copy link
Member Author

@JohnVillalovos JohnVillalovos Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@nejch nejch Jul 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try:
if name in self.__dict__["_updated_attrs"]:
return self.__dict__["_updated_attrs"][name]
except KeyError:
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:
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 exc

if name in self.__dict__["_attrs"]:
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 block.
if isinstance(value, list):
self.__dict__["_updated_attrs"][name] = value[:]
return self.__dict__["_updated_attrs"][name]

return value

if name in self.__dict__["_parent_attrs"]:
return self.__dict__["_parent_attrs"][name]

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)

def __setattr__(self, name: str, value: Any) -> None:
self.__dict__["_updated_attrs"][name] = value
Expand Down