Bump typeguard from 2.13.3 to 4.0.0 #655
Conversation
![dependabot[bot]](https://avatars.githubusercontent.com/in/29110?s=60&v=4){kind=small}
dependabot
bot
added
the
dependencies
Bumps [typeguard](https://github.com/agronholm/typeguard) from 2.13.3 to 4.0.0. - [Changelog](https://github.com/agronholm/typeguard/blob/master/docs/versionhistory.rst) - [Commits](agronholm/typeguard@2.13.3...4.0.0) --- updated-dependencies: - dependency-name: typeguard dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
- the error raised is a `TypeCheckError` - we need to enable the CollectionCheckStrategy.ALL_ITEMS to check each instance in a collection - instead of checking the generated error messages for each class we only check that an error is raised for every class and use only one invalid package as a proxy for all other tests to check the generated message Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
meretp
force-pushed
the
dependabot/pip/typeguard-4.0.0
branch
from
6a5b7f5 to
d26fd4e
Compare
|
To fix the tests I did the following:
The generated error message changed with the update. In the test_error_message module you can see an example for the new message. I think the part for package.download_location(so a wrong type where a Union of types is possible) takes a bit of getting used to, but in my opinion the problem becomes clear and we don't need to customize this error. |
| # As setters are created dynamically, their argument name is always "value". We replace it by the | ||
| # actual name so the error message is more helpful. | ||
| raise TypeError(error_message.replace("value", field_name, 1) + f": {value}") |
There was a problem hiding this comment.
Is there a particular reason why we don't print out the actual value anymore? I think this is quite useful to quickly pinpoint the source of the error.
There was a problem hiding this comment.
There are two main reasons for this change, but they might be a matter of taste:
- The "new" messages contain the type of the falsy value, so I thought of it as a duplicate, e.g.
'SetterError Package: argument "name" (None) is not an instance of str'
vs
'SetterError Package: argument "name" (None) is not an instance of str: None'
- If the type hint contains a
Unionthe error message is quite long and appending the real value doesn't really improve readability in my opinion,
e.g.
SetterError Package: argument "
'"download_location" (int) did not match any element in the union:\\n str: '
"is not an instance of str\\n "
"spdx_tools.spdx.model.spdx_no_assertion.SpdxNoAssertion: is not an instance "
"of spdx_tools.spdx.model.spdx_no_assertion.SpdxNoAssertion\\n "
"spdx_tools.spdx.model.spdx_none.SpdxNone: is not an instance of "
"spdx_tools.spdx.model.spdx_none.SpdxNone: 5'
There was a problem hiding this comment.
Hmm, your first point is only valid for None, isn't it? I'm thinking more of cases with actual values, like
'SetterError Package: argument "name" (int) is not an instance of str: 42'
Regarding your second point, I now believe that the Union error message might not be very good after all, as it is much too verbose for what it actually says and might now even lead us to drop much more interesting information.
With some fancy string manipulation we should be able to reduce the size for Union-like type checks.
There was a problem hiding this comment.
Well, it is "valid" in every case, so at least in my opinion also the example you provided feels like a duplication but might help the user in such a way that he or she can search for the actual value...
Alright let's do "fancy string manipulation"..Will then need to be adapted once the error messages are changed again but as this only affects one place in the code I think this is reasonable.
There was a problem hiding this comment.
I implemented the string manipulation and added the value back in again.
- simplify the error message for values with multiple possible types - add value to the error message Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
| 'argument "attribution_texts" (list) is not an instance of str\']' | ||
| "(None) is not an instance of str: None', 'SetterError Package: argument " | ||
| '"download_location" (int) did not match any element in the union: ' | ||
| "[\\'str\\', \\'spdx_tools.spdx.model.spdx_no_assertion.SpdxNoAssertion\\', " |
There was a problem hiding this comment.
I'd find it better not to have quotation marks around the types as this leads to escaped marks in the error output, which does not look good :)
|
|
||
| def simplify_error_message_for_union(error_message: str) -> str: | ||
| # The error message from typeguard is more verbose than we need, so we simplify the message | ||
| # to provide the user with a compacter error message. |
There was a problem hiding this comment.
| # to provide the user with a compacter error message. | |
| # to provide the user with a more compact error message. |
Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
Bumps typeguard from 2.13.3 to 4.0.0.
Changelog
Sourced from typeguard's changelog.
... (truncated)
Commits
887e27eDeclared the latest RC as final668d2a0Added release datebe4dd33Don't try to parse the second argument of Annotated as a forward reference3863c0fFixed AST transformations potentially leaving "if" bodies emptya2090ed[pre-commit.ci] pre-commit autoupdate (#351)a6a9b71Use the proper environment for releasec888515Added release date8923b7bMade@typecheckeda no-op in optimized mode (#350)43d686eSwitch to the official coveralls actione688da2Skip type checks where the type names are shadowed by local variablesDependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot mergewill merge this PR after your CI passes on it@dependabot squash and mergewill squash and merge this PR after your CI passes on it@dependabot cancel mergewill cancel a previously requested merge and block automerging@dependabot reopenwill reopen this PR if it is closed@dependabot closewill close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)