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

Fix type annotation on keyword argument in copy(**add_or_replace) #54

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

techsy730
Copy link
Contributor

@techsy730 techsy730 commented Jul 16, 2021

Fix type annotation on keyword argument in copy(**add_or_replace)

The type annotation for keyword arguments describes the type of the value, not of the kwargs as a whole itself. (See https://www.python.org/dev/peps/pep-0484/#id37)
This is because the keys of a keyword argument paramater is always str.

So the signature of of copy(self, **add_or_replace: Dict[_K, _V]) means "add_or_replace is a keyword arguments parameter whose values are of type Dict[_K, _V]", probably not what was intended.

Now it is set to be **add_or_replace: _V, which means it can accept keyword arguments of type _V

Incidentally, the return type annotation is incorrect if the self immutabledict didn't have Text keys or a supertype thereof (as now it is going to have Text keys in addition to whatever _K was), but that is a separate issue.

The type annotation for keyword arguments describes the type of the  _value_, not of the kwargs as a whole itself. (See https://www.python.org/dev/peps/pep-0484/#id37)
This is because the keys of a keyword argument paramater is always `str`.

So the signature of of `copy(self, **add_or_replace: Dict[_K, _V])` means "add_or_replace is a keyword arguments parameter whose _values_ are of type `Dict[_K, _V]`", probably not what was intended.

Incidentally, the return type annotation is incorrect if the self `immutabledict` didn't have `Text` keys or a supertype thereof, but that is a separate issue.
@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #54 (e599dc8) into master (a46e50c) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #54   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           51        51           
=========================================
  Hits            51        51           
Impacted Files Coverage Δ
immutabledict/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a46e50c...e599dc8. Read the comment docs.

@corenting
Copy link
Owner

Sorry for the issue and thanks for fixing the issue in your PR 👍

I will make a release ASAP to publish this fix.

@corenting corenting merged commit cb590b0 into corenting:master Jul 16, 2021
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.

None yet

2 participants