Jul 4, 2022 · Jun 30, 2022 · Jul 3, 2022 · Jul 4, 2022 · Jul 4, 2022
diff --git a/gitlab/base.py b/gitlab/base.py
        self.__dict__["_module"] = importlib.import_module(module_name)

    def __getattr__(self, name: str) -> Any:
 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
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 nejchJul 3, 2022 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:returnself.__dict__["_updated_attrs"][name]exceptKeyError:passtry: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.ifisinstance(value,list):self.__dict__["_updated_attrs"][name]=value[:]returnself.__dict__["_updated_attrs"][name]returnvalueexceptKeyError:passtry:returnself.__dict__["_parent_attrs"][name]exceptKeyErrorasexc:message= (f"{type(self).__name__!r} object has no attribute{name!r}" )ifself._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}" )raiseAttributeError(message)fromexc Not sure. Thoughts? Copy link MemberAuthor JohnVillalovosJul 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.helloTraceback (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 nejchJul 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