Skip to content
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.

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

Less strict dictionary merging in core #1507

Closed
wants to merge 1 commit into from

Conversation

yanMHG
Copy link

@yanMHG yanMHG commented Jan 17, 2024

In

https://github.com/fsspec/filesystem_spec/blob/7b774c26e30e5ae60b5b63bf4e345a1b32028a4e/fsspec/core.py#L337C9-L337C41

the dictionaries extra_kwargs and kws are merged with kw = dict(**extra_kwargs, **kws). This may be an issue when there is redundant information in the two dictionaries, resulting in a TypeError: dict() got multiple values for keyword argument (indeed I have just faced this exact situation in a project). I understand it is correct to raise this error when the same key is mapped to different values, but it is not necessary when the values are the same.

An alternative way to approach this merge is with kw = extra_kwargs | kws and raising the error only when there are overlapping keys with different values, something along the lines of

if any(k_1 == k_2 and v_1 != v_2 for k_1, v_1 in extra_kwargs.items() for k_2, v_2 in kws.items()):
    raise ValueError("Tried to merge dictionaries with same key and different values.")
else:
    kw = extra_kwargs | kws

@yanMHG yanMHG marked this pull request as ready for review January 17, 2024 21:16
@yanMHG yanMHG marked this pull request as draft January 19, 2024 06:19
@@ -334,7 +334,16 @@ def _un_chain(path, kwargs):
kws = kwargs.pop(protocol, {})
if bit is bits[0]:
kws.update(kwargs)
kw = dict(**extra_kwargs, **kws)
if any(
Copy link
Member

Choose a reason for hiding this comment

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

The simpler alternative would be

kw = extra_kwargs | kws

alone (you have this some lines below) without the check.

which does something different, but maybe the right thing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants