feat: optimize update_from_dict #75
Conversation
update_from_dict and _inject_api where always done together. Instead of iterating all the data multiple times, iterate it once and inject the api as we go
update_from_dict and _inject_api where always done together. Instead of iterating all the data multiple times, iterate it once and inject the api as we go
update_from_dict and _inject_api where always done together. Instead of iterating all the data multiple times, iterate it once and inject the api as we go
update_from_dict and _inject_api where always done together. Instead of iterating all the data multiple times, iterate it once and inject the api as we go
|
Warning Rate limit exceeded@bdraco has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 36 minutes and 37 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes in Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Codecov ReportAttention: Patch coverage is
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/uiprotect/data/base.py (1 hunks)
Additional context used
GitHub Check: codecov/patch
src/uiprotect/data/base.py
[warning] 515-515: src/uiprotect/data/base.py#L515
Added line #L515 was not covered by tests
[warning] 530-530: src/uiprotect/data/base.py#L530
Added line #L530 was not covered by tests
| unifi_obj = getattr(cls, key) | ||
| value = unifi_obj.update_from_dict(item) | ||
| elif has_unifi_lists and key in unifi_lists and isinstance(item, list): | ||
| klass = unifi_lists[key] |
There was a problem hiding this comment.
The line where a new instance of a class is created inside a list comprehension was not covered by tests. This is a crucial part of the functionality as it involves data transformation and API injection. It is recommended to add unit tests to cover this scenario to ensure that the creation and initialization of objects in lists are handled correctly.
Would you like assistance in writing these unit tests?
Tools
GitHub Check: codecov/patch
[warning] 515-515: src/uiprotect/data/base.py#L515
Added line #L515 was not covered by tests
src/uiprotect/data/base.py
Outdated
| ): | ||
| for i in item.values(): | ||
| if isinstance(i, dict): | ||
| i["api"] = api |
There was a problem hiding this comment.
Similar to the previous comment, this line where the API client is injected into dictionary values was not covered by tests. Given that this is a key part of the API injection logic, comprehensive testing is essential to ensure that the API client is correctly and safely injected into all applicable objects.
I can help draft these unit tests or open a GitHub issue to track this task. Let me know how you would like to proceed.
Tools
GitHub Check: codecov/patch
[warning] 530-530: src/uiprotect/data/base.py#L530
Added line #L530 was not covered by tests
src/uiprotect/data/base.py
Outdated
| def update_from_dict(cls: ProtectObject, data: dict[str, Any]) -> ProtectObject: | ||
| """ | ||
| Updates current object from a cleaned UFP JSON dict. | ||
|
|
||
| return data | ||
| The api client is injected into each dict for any child | ||
| UFP objects that are detected. | ||
| """ | ||
| unifi_objs = cls._get_protect_objs() | ||
| has_unifi_objs = bool(unifi_objs) | ||
| unifi_lists = cls._get_protect_lists() | ||
| has_unifi_lists = bool(unifi_lists) | ||
| unifi_dicts_sets = cls._get_protect_dicts_set() | ||
| has_unifi_dicts = bool(unifi_dicts_sets) | ||
|
|
||
| def update_from_dict(self: ProtectObject, data: dict[str, Any]) -> ProtectObject: | ||
| """Updates current object from a cleaned UFP JSON dict""" | ||
| data_set = set(data) | ||
| for key in self._get_protect_objs_set().intersection(data_set): | ||
| unifi_obj: Any | None = getattr(self, key) | ||
| if unifi_obj is not None and isinstance(unifi_obj, ProtectBaseObject): | ||
| item = data.pop(key) | ||
| if item is not None: | ||
| item = unifi_obj.update_from_dict(item) | ||
| setattr(self, key, item) | ||
|
|
||
| data = self._inject_api(data, self._api) | ||
| unifi_lists = self._get_protect_lists() | ||
| for key in self._get_protect_lists_set().intersection(data_set): | ||
| if not isinstance(data[key], list): | ||
| continue | ||
| klass = unifi_lists[key] | ||
| new_items = [] | ||
| for item in data.pop(key): | ||
| if item is not None and isinstance(item, ProtectBaseObject): | ||
| new_items.append(item) | ||
| elif isinstance(item, dict): | ||
| new_items.append(klass(**item)) | ||
| setattr(self, key, new_items) | ||
|
|
||
| # Always injected above | ||
| del data["api"] | ||
|
|
||
| for key in data: | ||
| setattr(self, key, convert_unifi_data(data[key], self.__fields__[key])) | ||
|
|
||
| return self | ||
| api = cls._api | ||
| _fields = cls.__fields__ | ||
| unifi_obj: ProtectBaseObject | ||
| if "api" in data: | ||
| del data["api"] | ||
| value: Any | ||
|
|
||
| for key, item in data.items(): | ||
| if has_unifi_objs and key in unifi_objs and isinstance(item, dict): | ||
| item["api"] = api | ||
| unifi_obj = getattr(cls, key) | ||
| value = unifi_obj.update_from_dict(item) | ||
| elif has_unifi_lists and key in unifi_lists and isinstance(item, list): | ||
| klass = unifi_lists[key] | ||
| value = [ | ||
| klass(**i, api=api) if isinstance(i, dict) else i | ||
| for i in item | ||
| if i is not None and isinstance(i, (dict, ProtectBaseObject)) | ||
| ] | ||
| else: | ||
| # Inject the api if the key is in the unifi_dicts_sets | ||
| if ( | ||
| has_unifi_dicts | ||
| and key in unifi_dicts_sets | ||
| and isinstance(item, dict) | ||
| ): | ||
| for i in item.values(): | ||
| if isinstance(i, dict): | ||
| i["api"] = api | ||
| value = convert_unifi_data(item, _fields[key]) | ||
|
|
||
| setattr(cls, key, value) | ||
|
|
||
| return cls |
There was a problem hiding this comment.
The refactored update_from_dict method integrates API injection directly into the data iteration process, which should indeed optimize performance by reducing the number of iterations. However, there are several areas that could be further improved for clarity and efficiency:
- The method uses
getattr(cls, key)which might raise anAttributeErrorifkeydoes not actually correspond to an attribute ofcls. It might be safer to usegetattr(cls, key, None)and check if the result isNonebefore proceeding. - The deletion of the 'api' key directly in the data dictionary could lead to side-effects if the data dictionary is used elsewhere after this method call. It might be better to work with a copy of the data.
- The method does not handle potential exceptions from
convert_unifi_data, which might raise errors if the data is not in the expected format.
Consider adding error handling and working with a copy of the data to avoid side-effects.
+ data = data.copy() # Work with a copy to avoid side-effects
+ try:
+ if unifi_obj is None:
+ continue # Skip if the attribute does not exist
+ except Exception as e:
+ _LOGGER.error("Failed to update from dict: %s", e)
+ raiseCommittable suggestion was skipped due to low confidence.
Tools
GitHub Check: codecov/patch
[warning] 515-515: src/uiprotect/data/base.py#L515
Added line #L515 was not covered by tests
[warning] 530-530: src/uiprotect/data/base.py#L530
Added line #L530 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/uiprotect/data/base.py (1 hunks)
Additional comments not posted (2)
src/uiprotect/data/base.py (2)
488-537: Update method logic to handle API injection and data updating in one pass.This refactoring to combine
_inject_apiwithupdate_from_dictis a good optimization. It reduces redundancy and potential errors by handling everything in a single loop. However, consider adding error handling for cases wherekeymight not exist in_fields, which could raise a KeyError.
537-537: Verify integration and functionality with other class methods.Ensure that the changes in
update_from_dictdo not adversely affect other methods in the class, such asdict_with_excludesandget_changed. It would be beneficial to conduct integration testing to confirm that these methods still perform as expected with the new update logic.Also applies to: 488-488
src/uiprotect/data/base.py
Outdated
| for key, item in data.items(): | ||
| if has_unifi_objs and key in unifi_objs and isinstance(item, dict): | ||
| if (unifi_obj := getattr(cls, key)) is not None: | ||
| item["api"] = api | ||
| value = unifi_obj.update_from_dict(item) | ||
| else: | ||
| value = None | ||
| elif has_unifi_lists and key in unifi_lists and isinstance(item, list): | ||
| klass = unifi_lists[key] | ||
| value = [ | ||
| klass(**i, api=api) if isinstance(i, dict) else i | ||
| for i in item | ||
| if i is not None and isinstance(i, (dict, ProtectBaseObject)) | ||
| ] | ||
| else: | ||
| # Inject the api if the key is in the unifi_dicts_sets | ||
| if ( | ||
| has_unifi_dicts | ||
| and key in unifi_dicts_sets | ||
| and isinstance(item, dict) | ||
| ): | ||
| for i in item.values(): | ||
| if isinstance(i, dict): | ||
| i["api"] = api | ||
| value = convert_unifi_data(item, _fields[key]) | ||
|
|
||
| setattr(cls, key, value) |
There was a problem hiding this comment.
Ensure robust error handling and data integrity during the update process.
Consider adding a check to ensure that key exists in _fields before attempting to access it, which would prevent potential KeyError exceptions. Also, consider wrapping the update logic in a try-except block to handle unexpected issues during the update process gracefully.
+ try:
+ if key not in _fields:
+ continue # Skip if the field does not exist
+ # Existing update logic here...
+ except Exception as e:
+ _LOGGER.error("Failed to update from dict: %s", e)
+ raiseCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for key, item in data.items(): | |
| if has_unifi_objs and key in unifi_objs and isinstance(item, dict): | |
| if (unifi_obj := getattr(cls, key)) is not None: | |
| item["api"] = api | |
| value = unifi_obj.update_from_dict(item) | |
| else: | |
| value = None | |
| elif has_unifi_lists and key in unifi_lists and isinstance(item, list): | |
| klass = unifi_lists[key] | |
| value = [ | |
| klass(**i, api=api) if isinstance(i, dict) else i | |
| for i in item | |
| if i is not None and isinstance(i, (dict, ProtectBaseObject)) | |
| ] | |
| else: | |
| # Inject the api if the key is in the unifi_dicts_sets | |
| if ( | |
| has_unifi_dicts | |
| and key in unifi_dicts_sets | |
| and isinstance(item, dict) | |
| ): | |
| for i in item.values(): | |
| if isinstance(i, dict): | |
| i["api"] = api | |
| value = convert_unifi_data(item, _fields[key]) | |
| setattr(cls, key, value) | |
| for key, item in data.items(): | |
| try: | |
| if key not in _fields: | |
| continue # Skip if the field does not exist | |
| if has_unifi_objs and key in unifi_objs and isinstance(item, dict): | |
| if (unifi_obj := getattr(cls, key)) is not None: | |
| item["api"] = api | |
| value = unifi_obj.update_from_dict(item) | |
| else: | |
| value = None | |
| elif has_unifi_lists and key in unifi_lists and isinstance(item, list): | |
| klass = unifi_lists[key] | |
| value = [ | |
| klass(**i, api=api) if isinstance(i, dict) else i | |
| for i in item | |
| if i is not None and isinstance(i, (dict, ProtectBaseObject)) | |
| ] | |
| else: | |
| # Inject the api if the key is in the unifi_dicts_sets | |
| if ( | |
| has_unifi_dicts | |
| and key in unifi_dicts_sets | |
| and isinstance(item, dict) | |
| ): | |
| for i in item.values(): | |
| if isinstance(i, dict): | |
| i["api"] = api | |
| value = convert_unifi_data(item, _fields[key]) | |
| setattr(cls, key, value) | |
| except Exception as e: | |
| _LOGGER.error("Failed to update from dict: %s", e) | |
| raise |
Description of change
update_from_dict and _inject_api where always done together.
Instead of iterating all the data multiple times, iterate it
once and inject the api as we go
Summary by CodeRabbit