Skip to content

revert some changes to TypedDict which crept in via Flux #449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mtitov merged 2 commits into from
Oct 31, 2025
Merged

revert some changes to TypedDict which crept in via Flux #449

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b4f403
revert some changes to `TypedDict` which crept in via Flux
andre-merzky Oct 31, 2025
3aec843
Merge branch 'devel' into revert/tdict
mtitov Oct 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 7 additions & 13 deletions src/radical/utils/typeddict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,13 +158,11 @@ def __init__(self, from_dict=None, **kwargs):


if self._deep:
self.__dict__['_data'] = copy.deepcopy(self._defaults)
self.update(copy.deepcopy(self._defaults))
else:
self.__dict__['_data'] = dict()
self.__dict__['_data'].update(self._defaults)
self.update(self._defaults)

if from_dict:
self.update(from_dict)
self.update(from_dict)
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling self.update(from_dict) when from_dict is None will cause an AttributeError since None does not have an items() method. While the update() method checks if not other:, this will only skip None and empty dicts. However, the call is unconditional here. Consider adding a conditional check before calling update(from_dict), similar to how kwargs is handled on line 167.

Suggested change
self.update(from_dict)
if from_dict is not None:
self.update(from_dict)

Copilot uses AI. Check for mistakes.

if kwargs:
self.update(kwargs)
Expand Down Expand Up @@ -230,10 +228,7 @@ def __getitem__(self, k):
return self._data[k]

def __setitem__(self, k, v):
if self._check :
self._data[k] = self._verify_setter(k, v)
else:
self._data[k] = v
self._data[k] = self._verify_setter(k, v)
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The _verify_setter method at line 461 returns v unchanged when _check is False or when there is no _schema. By removing the if self._check: conditional here and always calling _verify_setter, the behavior is preserved for cases where _check is True. However, this now adds an extra function call overhead even when _check is False. While functionally correct, this could be a minor performance regression for the common case where _check is False (the default). Consider whether this performance tradeoff is acceptable.

Suggested change
self._data[k] = self._verify_setter(k, v)
if self._check:
self._data[k] = self._verify_setter(k, v)
else:
self._data[k] = v

Copilot uses AI. Check for mistakes.

def __delitem__(self, k):
del self._data[k]
Expand Down Expand Up @@ -300,6 +295,8 @@ def popitem(self):
def __getattr__(self, k):

if k == '_data':
if '_data' not in self.__dict__:
self.__dict__['_data'] = dict()
return self.__dict__['_data']

if k.startswith('__'):
Expand All @@ -317,10 +314,7 @@ def __setattr__(self, k, v):
if k.startswith('__'):
return object.__setattr__(self, k, v)

if self._check :
self._data[k] = self._verify_setter(k, v)
else:
self._data[k] = v
self._data[k] = self._verify_setter(k, v)

def __delattr__(self, k):

Expand Down