Adds covariant true to key type #244
Conversation
Codecov Report
@@ Coverage Diff @@
## master #244 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 49 49
=========================================
Hits 49 49
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Changing variance will not be a breaking change - the worst effect it may have is breaking someone However, removing or changing |
| @@ -4,6 +4,10 @@ | |||
|
|
|||
|
|
|||
| class TestImmutableDict: | |||
| def test_covariance(self): | |||
| assert immutabledict.__parameters__[0].__covariant__ is False | |||
There was a problem hiding this comment.
Indeed, testing typing behaviour at runtime is rarely useful. Such things are usually tested by interaction and not by implementation: you can add a code file like
foo: immutabledict[str, str]
bar: immutabledict[str, str | int] = foo
and run mypy on it (considering other usage scenarios as well, of course, and using .copy there with different scenarios if you choose to keep/update it instead of removing to make sure it works as intended). Alternatively, have such lines somewhere in tests and run type checking on the test suite (some projects do this and live happily, though maintaining correct typing in tests looks like unnecessary overhead to me personally).
There was a problem hiding this comment.
If immutabledict itself is type-checked in CI, you could adopt this approach where you add a file (excluded from the dist) where "try out" the library.
It's both easier to understand and more "direct" than asserting on __parameters__[0].__covariant__ :)
There was a problem hiding this comment.
While I agree that that would be a nicer way to test this, CI does not currently have any type checking and
my current test ensures that the feature is working
There was a problem hiding this comment.
Then I'd just avoid testing this rather than test typing internals. It also doesn't prove the end result.
There was a problem hiding this comment.
You and I disagree here
- the feature that this is adding is making the key parameter covariant Removing the test would remove this future failure signal and could break future type hints.
- the test verifies that that is the case and that that typevar is used for the key. If a developer changes this later the test will fail.
- mypy and other type checkers implement covariance so while it would be nice to check it, a check that mypy implements covariance using immutabledict is not strictly required
If others want to add a mypy run + test to this PR or in a future PR that is fine by me
There was a problem hiding this comment.
Don't worry for the tests.
I think there are some other unrelated issues for the typing of the ordered immutabledict, I'm gonna work on it and use the opportunity to add better ci checks for the typing.
spacether
force-pushed
the
feat_adds_covariance
branch
from
adb9b6d
to
4141169
Compare
|
Thanks again for the PR @spacether ! |
|
You're welcome. Thanks for making the package and for the review! |
Adds covariant true to key
This will allow this class to behave the same way that other immutable containers do.
If approved, this will close out this issue: #243
One should probably make this a major release as adding covariant probably makes this a breaking change. Typing experts please weigh in if you see this.