Fix parsing issues in TextStyle and *Event classes
#3551
Conversation
Reviewer's Guide by SourceryThis pull request addresses and fixes issues related to File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ndonkoHenri - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
ndonkoHenri
changed the title
TextStyle parsing issuesTextStyle and *Event classes
There was a problem hiding this comment.
Hey @ndonkoHenri - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 7 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| self.local_x: float = d.get("lx") | ||
| self.local_y: float = d.get("ly") | ||
| self.global_x: float = d.get("gx") | ||
| self.global_y: float = d.get("gy") | ||
| self.kind: str = d.get("kind") |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with default values
Using d.get() will return None if the key is not found. This might cause issues if the attributes are expected to be non-null. Consider providing default values to d.get(), e.g., d.get('lx', 0.0).
| self.local_y: float = d.get("ly") | ||
| self.global_x: float = d.get("gx") | ||
| self.global_y: float = d.get("gy") | ||
| self.timestamp: Optional[int] = d.get("ts") |
There was a problem hiding this comment.
suggestion: Inconsistent use of Optional
The use of Optional is inconsistent across the code. For example, self.timestamp is marked as Optional[int] but self.local_x is not marked as Optional[float]. Consider making the use of Optional consistent.
| self.timestamp: Optional[int] = d.get("ts") | |
| self.local_y: Optional[float] = d.get("ly") | |
| self.global_x: Optional[float] = d.get("gx") | |
| self.global_y: Optional[float] = d.get("gy") | |
| self.timestamp: Optional[int] = d.get("ts") |
| self.coordinates: MapLatitudeLongitude = MapLatitudeLongitude( | ||
| d["lat"], d["long"] | ||
| d.get("lat"), d.get("long") |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with default values in coordinates
If d.get('lat') or d.get('long') returns None, it might cause issues in MapLatitudeLongitude. Consider providing default values or handling None cases.
| @@ -19,6 +19,7 @@ def __init__( | |||
| on_scroll_interval: OptionalNumber = None, | |||
| on_scroll: Optional[Callable[["OnScrollEvent"], None]] = None, | |||
| ): | |||
| Control.__init__(self) | |||
There was a problem hiding this comment.
suggestion: Redundant call to superclass constructor
The call to Control.__init__(self) is redundant if super().__init__() is already being called in the constructor. Consider removing it to avoid confusion.
| Control.__init__(self) | |
| super().__init__() |
There was a problem hiding this comment.
@sourcery-ai explain this point more detaily
There was a problem hiding this comment.
Sure, I'd be happy to explain this in more detail.
In Python, when you define a class that inherits from a superclass, you often need to call the constructor of the superclass to ensure that the superclass is properly initialized. This is typically done using the super() function.
For example, if you have a class Control and a subclass ScrollableControl, you would call the superclass constructor like this:
class ScrollableControl(Control):
def __init__(self, ...):
super().__init__()
# additional initialization for ScrollableControlThe super().__init__() call ensures that the __init__ method of the Control class is executed, which sets up any necessary state or properties defined in Control.
In the diff hunk you provided, there is a call to Control.__init__(self):
class ScrollableControl(Control):
def __init__(self, ...):
Control.__init__(self)
# additional initialization for ScrollableControlThis is a more explicit way of calling the superclass constructor, but it is generally considered less flexible and less idiomatic in modern Python code. The super() function is preferred because it automatically handles the correct method resolution order, especially in cases of multiple inheritance.
So, the suggestion to replace Control.__init__(self) with super().__init__() is to make the code more idiomatic and to avoid potential issues with method resolution order in more complex inheritance hierarchies.
Here's the revised code with the suggested change:
class ScrollableControl(Control):
def __init__(self, ...):
super().__init__()
# additional initialization for ScrollableControlThis way, the superclass constructor is called in a more standard and flexible manner.
| self.direction: DismissDirection = DismissDirection(d.get("direction")) | ||
|
|
||
|
|
||
| class DismissibleUpdateEvent(ControlEvent): | ||
| def __init__(self, e: ControlEvent): | ||
| super().__init__(e.target, e.name, e.data, e.control, e.page) | ||
| d = json.loads(e.data) | ||
| self.direction = DismissDirection(d["direction"]) | ||
| self.progress = d["progress"] | ||
| self.reached = d["reached"] | ||
| self.previous_reached = d["previous_reached"] | ||
| self.direction: DismissDirection = DismissDirection(d.get("direction")) | ||
| self.progress: float = d.get("progress") | ||
| self.reached: bool = d.get("reached") |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with default values in DismissibleUpdateEvent
Using d.get() without default values might cause issues if the keys are not present. Consider providing default values or handling None cases.
| @@ -1116,7 +1116,7 @@ async def _authorize_callback_async(self, data: str) -> None: | |||
| async def __on_authorize_async(self, e) -> None: | |||
| assert self.__authorization | |||
| d = json.loads(e.data) | |||
| state = d["state"] | |||
| state = d.get("state") | |||
There was a problem hiding this comment.
issue (bug_risk): Potential issue with state validation
If d.get('state') returns None, the subsequent assert might fail. Consider handling the None case explicitly.
| self.column_index: int = d.get("i") | ||
| self.ascending: bool = d.get("a") |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with default values in DataColumnSortEvent
Using d.get() without default values might cause issues if the keys are not present. Consider providing default values or handling None cases.
| self.src_id: float = d["src_id"] | ||
| self.x: float = d["x"] | ||
| self.y: float = d["y"] | ||
| self.src_id: float = d.get("src_id") |
There was a problem hiding this comment.
issue (complexity): Consider explicitly handling missing keys instead of using get.
The new code introduces additional complexity by using the get method for dictionary access, which can silently assign None to attributes if keys are missing. This can mask issues and make debugging harder. The original code's direct dictionary access would raise a KeyError, providing a clear signal when expected keys are missing. If handling missing keys is necessary, it should be done explicitly with proper error handling. Here's a suggested approach:
class DragTargetAcceptEvent(ControlEvent):
def __init__(self, e: ControlEvent):
super().__init__(e.target, e.name, e.data, e.control, e.page)
d = json.loads(e.data)
warn(
f"{self.__class__.__name__} is deprecated since version 0.22.0 "
f"and will be removed in version 0.26.0. Use DragTargetEvent instead.",
category=DeprecationWarning,
stacklevel=2,
)
try:
self.src_id: float = d["src_id"]
self.x: float = d["x"]
self.y: float = d["y"]
except KeyError as key_error:
raise ValueError(f"Missing expected key in data: {key_error}")
class DragTargetEvent(ControlEvent):
def __init__(self, e: ControlEvent):
super().__init__(e.target, e.name, e.data, e.control, e.page)
d = json.loads(e.data)
try:
self.src_id: float = d["src_id"]
self.x: float = d["x"]
self.y: float = d["y"]
except KeyError as key_error:
raise ValueError(f"Missing expected key in data: {key_error}")This maintains simplicity while explicitly handling missing keys, making the code more robust and easier to maintain.
Fixes #3532
Summary by Sourcery
This pull request addresses parsing issues in multiple event classes by using the
getmethod for dictionary access to handle missing keys. Additionally, it refactors theme parsing functions to usetextStyleFromJsondirectly, enhancing code consistency.getmethod to handle missing keys gracefully.parseTextStylefunction usage in theme parsing functions to usetextStyleFromJsondirectly, improving code consistency.