kiwiz / gkeepapi

An unofficial client for the Google Keep API.
MIT License
1.54k stars 114 forks source link

Node.sort: return an int, not a string of an int #117

Closed wizpig64 closed 2 years ago

wizpig64 commented 2 years ago

Hello,

Currently, the Node.sort property returns strings of integers, like '1234'. This PR makes it always return an int, which is how it's annotated:

@@ -1072,7 +1072,7 @@ class Node(Element, TimestampsMixin):
         Returns:
             int: Sort id.
         """
-        return self._sort
+        return int(self._sort)

     @sort.setter
     def sort(self, value):

Longer Explanation

Here's the existing Node.sort property: https://github.com/kiwiz/gkeepapi/blob/master/gkeepapi/node.py#L1068-L1080

    @property
    def sort(self):
        """Get the sort id.
        Returns:
            int: Sort id.
        """
        return self._sort

    @sort.setter
    def sort(self, value):
        self._sort = value
        self.touch()

The getter declares itself to always return an integer, but in my experience it returns strings of integers like '1230000999', probably due to a javascriptism in keep's server-side stack. This means I have to cast it as an integer myself if I want to manipulate it. Setting an integer string on this property works as expected, as the upstream API knows how to handle that.

However, gkeepapi's implementation of List.add() really does require an integer, as it checks isinstance(sort, int) before setting the property: https://github.com/kiwiz/gkeepapi/blob/master/gkeepapi/node.py#L1368

    def add(self, text, checked=False, sort=None):
        """Add a new item to the list.

        Args:
            text (str): The text.
            checked (bool): Whether this item is checked.
            sort (Union[gkeepapi.node.NewListItemPlacementValue, int]): Item id for sorting or a placement policy.
        """
        node = ListItem(...)
        ...
        if isinstance(sort, int):
            node.sort = sort
        elif isinstance(sort, NewListItemPlacementValue) and len(items):
            node.sort = ...

        self.append(node, True)
        self.touch(True)
        return node

Given this implementation, if you call List.add(other_item.text, sort=other_item.sort), using a string sort value provided by the API, it will be equivalent to calling it with sort=None, putting the new item on the very top of the list, and it won't give you any error telling you not to use a string.

This change prevents sort values from entering the python side as strings in the first place, so I figured .add() could be left alone.

Disaster Mitigation

This change also might introduce the possibility of a TypeError if ._sort ever returns None, or ValueError if it's a string that can't be converted into an int. I don't have any proof that it would ever do that, but to ease my superstitions, here's the patch I would apply on top of this one if I wanted to prevent such errors:

@@ -1072,7 +1073,9 @@ class Node(Element, TimestampsMixin):
         Returns:
             int: Sort id.
         """
-        return int(self._sort)
+        import contextlib.suppress
+        with contextlib.suppress(TypeError, ValueError):
+            return int(self._sort)
+        return self._sort

     @sort.setter
     def sort(self, value):
kiwiz commented 2 years ago

Looks reasonable, thanks. I don't expect _sort to ever be undef, but we'll cross that bridge ~when~ if we get to it.

kiwiz commented 2 years ago

Uploaded v0.13.7.