Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces native BSON data type support (such as BSONObjectID, BSONDecimal128, BSONTimestamp, BSONRegex, BSONBinary, BSONInt32, BSONMinKey, and BSONMaxKey) to the Firestore Python SDK, along with corresponding unit and system tests. The review feedback focuses on improving robustness and PEP 8 compliance. Key recommendations include using isinstance instead of direct type comparisons to support integer subclasses, safely retrieving attributes using getattr and callable rather than hasattr, correctly converting integer regex flags to option characters when a compiled regex is passed, and supporting direct equality comparisons between BSONDecimal128 and standard decimal.Decimal objects.
| if hasattr(pattern, "pattern") and not isinstance(pattern, str): | ||
| flags = getattr(pattern, "flags", "") | ||
| options = flags if isinstance(flags, str) else options | ||
| pattern = getattr(pattern, "pattern") |
There was a problem hiding this comment.
When a compiled regex object is passed, its flags attribute is an integer (e.g., re.IGNORECASE). The current implementation silently ignores integer flags, which drops critical regex options like case-insensitivity. Convert integer flags to their corresponding BSON regex option characters.
pattern_attr = getattr(pattern, "pattern", None)
if pattern_attr is not None and not isinstance(pattern, str):
flags = getattr(pattern, "flags", "")
if isinstance(flags, int):
opts = []
if flags & re.IGNORECASE:
opts.append("i")
if flags & re.MULTILINE:
opts.append("m")
if flags & re.DOTALL:
opts.append("s")
if flags & re.VERBOSE:
opts.append("x")
if flags & re.LOCALE:
opts.append("l")
if flags & re.UNICODE:
opts.append("u")
options = "".join(opts)
elif isinstance(flags, str):
options = flags
pattern = pattern_attr| if hasattr(value, "to_map_value"): | ||
| return encode_value(value.to_map_value()) |
There was a problem hiding this comment.
Using hasattr to check for to_map_value can be unsafe if the attribute is not callable (e.g., if it is a property or a mock). It is safer and more robust to use callable(getattr(value, 'to_map_value', None)).
| if hasattr(value, "to_map_value"): | |
| return encode_value(value.to_map_value()) | |
| to_map_value = getattr(value, "to_map_value", None) | |
| if callable(to_map_value): | |
| return encode_value(to_map_value()) |
| if hasattr(value, "binary") and isinstance(getattr(value, "binary"), (bytes, bytearray, memoryview)): | ||
| value = bytes(getattr(value, "binary")) | ||
| elif hasattr(value, "value") and isinstance(getattr(value, "value"), str): | ||
| value = getattr(value, "value") |
There was a problem hiding this comment.
Simplify attribute lookups using getattr with a default value. This avoids multiple lookups and redundant hasattr checks, making the initialization cleaner and more efficient.
binary_attr = getattr(value, "binary", None)
if isinstance(binary_attr, (bytes, bytearray, memoryview)):
value = bytes(binary_attr)
else:
value_attr = getattr(value, "value", None)
if isinstance(value_attr, str):
value = value_attr| if hasattr(value, "to_decimal") and callable(getattr(value, "to_decimal")): | ||
| value = getattr(value, "to_decimal")() |
There was a problem hiding this comment.
Simplify the check and lookup for to_decimal using getattr with a default value to avoid redundant lookups.
| if hasattr(value, "to_decimal") and callable(getattr(value, "to_decimal")): | |
| value = getattr(value, "to_decimal")() | |
| to_decimal_attr = getattr(value, "to_decimal", None) | |
| if callable(to_decimal_attr): | |
| value = to_decimal_attr() |
| if isinstance(other, BSONDecimal128): | ||
| try: | ||
| d1 = self.to_decimal() | ||
| d2 = other.to_decimal() | ||
| if d1.is_nan() or d2.is_nan(): | ||
| return False | ||
| return d1 == d2 | ||
| except (decimal.DecimalException, ArithmeticError): | ||
| return self._value == other._value | ||
| return False |
There was a problem hiding this comment.
For consistency with BSONInt32 (which supports equality with standard int), BSONDecimal128 should support direct equality comparison with decimal.Decimal.
if isinstance(other, BSONDecimal128):
try:
d1 = self.to_decimal()
d2 = other.to_decimal()
if d1.is_nan() or d2.is_nan():
return False
return d1 == d2
except (decimal.DecimalException, ArithmeticError):
return self._value == other._value
elif isinstance(other, decimal.Decimal):
try:
d1 = self.to_decimal()
if d1.is_nan() or other.is_nan():
return False
return d1 == other
except (decimal.DecimalException, ArithmeticError):
return False
return False| if type(seconds) is not int or type(increment) is not int: | ||
| raise TypeError("seconds and increment must be ints.") |
There was a problem hiding this comment.
Comparing types directly using type(seconds) is not int violates PEP 8 guidelines and rejects valid subclasses of int (such as IntEnum). Use isinstance instead, while explicitly rejecting bool.
| if type(seconds) is not int or type(increment) is not int: | |
| raise TypeError("seconds and increment must be ints.") | |
| if (not isinstance(seconds, int) or isinstance(seconds, bool) or | |
| not isinstance(increment, int) or isinstance(increment, bool)): | |
| raise TypeError("seconds and increment must be ints.") |
References
- PEP 8: Object type comparisons should always use isinstance() instead of comparing types directly. (link)
| if isinstance(subtype, bool) or type(subtype) is not int: | ||
| raise TypeError("subtype must be an integer.") |
There was a problem hiding this comment.
Comparing types directly using type(subtype) is not int violates PEP 8 guidelines and rejects valid subclasses of int. Use isinstance instead, while explicitly rejecting bool.
| if isinstance(subtype, bool) or type(subtype) is not int: | |
| raise TypeError("subtype must be an integer.") | |
| if not isinstance(subtype, int) or isinstance(subtype, bool): | |
| raise TypeError("subtype must be an integer.") |
References
- PEP 8: Object type comparisons should always use isinstance() instead of comparing types directly. (link)
| if hasattr(value, "value") and type(getattr(value, "value")) is int: | ||
| value = getattr(value, "value") | ||
| if type(value) is not int and not isinstance(value, (int, BSONInt32)): | ||
| raise TypeError("BSONInt32 value must be an integer.") |
There was a problem hiding this comment.
Simplify attribute lookups and type checking using getattr and isinstance to conform to PEP 8 guidelines and support subclasses of int.
| if hasattr(value, "value") and type(getattr(value, "value")) is int: | |
| value = getattr(value, "value") | |
| if type(value) is not int and not isinstance(value, (int, BSONInt32)): | |
| raise TypeError("BSONInt32 value must be an integer.") | |
| val_attr = getattr(value, "value", None) | |
| if val_attr is not None and isinstance(val_attr, int) and not isinstance(val_attr, bool): | |
| value = val_attr | |
| if not isinstance(value, (int, BSONInt32)): | |
| raise TypeError("BSONInt32 value must be an integer.") |
References
- PEP 8: Object type comparisons should always use isinstance() instead of comparing types directly. (link)
| if type(other) is int and not isinstance(other, bool): | ||
| return self._value == other |
There was a problem hiding this comment.
Comparing types directly using type(other) is int violates PEP 8 guidelines and rejects valid subclasses of int (such as IntEnum). Use isinstance instead.
| if type(other) is int and not isinstance(other, bool): | |
| return self._value == other | |
| if isinstance(other, int) and not isinstance(other, bool): | |
| return self._value == other |
References
- PEP 8: Object type comparisons should always use isinstance() instead of comparing types directly. (link)
35f2aab to
09c273b
Compare
09c273b to
209f604
Compare
1 participant
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #<issue_number_goes_here> 🦕