Skip to content

Bump typeguard from 2.13.3 to 4.0.0 #655

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
merged 4 commits into from
May 16, 2023
Merged

Bump typeguard from 2.13.3 to 4.0.0 #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f877b7
Bump typeguard from 2.13.3 to 4.0.0
dependabot[bot] May 15, 2023
d26fd4e
fix tests due to upgrade of typeguard
meretp May 15, 2023
05b6f7d
implement review comments
meretp May 16, 2023
719dd58
implement review comments
meretp May 16, 2023
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
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ classifiers = [
]
urls = { Homepage = "https://github.com/spdx/tools-python" }
requires-python = ">=3.7"
dependencies = ["click", "pyyaml", "xmltodict", "rdflib", "typeguard==2.13.3", "uritools", "license_expression", "ply"]
dependencies = ["click", "pyyaml", "xmltodict", "rdflib", "typeguard==4.0.0", "uritools", "license_expression", "ply"]
dynamic = ["version"]

[project.optional-dependencies]
Expand Down
25 changes: 20 additions & 5 deletions src/spdx_tools/common/typing/dataclass_with_properties.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import re
from dataclasses import dataclass

from typeguard import typechecked
from typeguard import CollectionCheckStrategy, TypeCheckError, config, typechecked

config.collection_check_strategy = CollectionCheckStrategy.ALL_ITEMS


def dataclass_with_properties(cls):
Expand All @@ -26,12 +29,24 @@ def set_field(self, value: field_type):
def set_field_with_better_error_message(self, value: field_type):
try:
set_field(self, value)
except TypeError as err:
error_message: str = f"SetterError {self.__class__.__name__}: {err.args[0]}"
except TypeCheckError as err:
error_message: str = f"SetterError {self.__class__.__name__}: {err}"
if "did not match any element in the union" in error_message:
error_message = simplify_error_message_for_union(error_message)

# 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}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 Union the 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'

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I implemented the string manipulation and added the value back in again.


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 more compact error message.
types_in_union = re.compile(r"\n\s*(.*?):", re.UNICODE)
list_of_types = re.findall(types_in_union, error_message)
text_to_replace = error_message.split("did not match any element in the union:")[-1]
error_message = error_message.replace(text_to_replace, " [" + ", ".join(list_of_types) + "]")
return error_message

return set_field_with_better_error_message


Expand All @@ -45,8 +60,8 @@ def get_field(self) -> field_type:
def get_field_with_better_error_message(self) -> field_type:
try:
return get_field(self)
except TypeError as err:
error_message: str = f"GetterError {self.__class__.__name__}: {err.args[0]}"
except TypeCheckError as err:
error_message: str = f"GetterError {self.__class__.__name__}: {err}"
# As getters are created dynamically, their argument name is always "the return value".
# We replace it by the actual name so the error message is more helpful.
raise TypeError(
Expand Down
36 changes: 5 additions & 31 deletions tests/spdx/parser/jsonlikedict/test_annotation_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,40 +119,14 @@ def test_parse_all_annotations():


@pytest.mark.parametrize(
"incomplete_annotation_dict,expected_message",
"incomplete_annotation_dict",
[
(
{"annotator": "Person: Jane Doe ()"},
[
"Error while constructing Annotation: ['SetterError Annotation: type of "
'argument "spdx_id" must be str; got NoneType instead: None\', '
'\'SetterError Annotation: type of argument "annotation_type" must be '
"spdx_tools.spdx.model.annotation.AnnotationType; got NoneType instead: None', "
'\'SetterError Annotation: type of argument "annotation_date" must be '
"datetime.datetime; got NoneType instead: None', 'SetterError Annotation: "
'type of argument "annotation_comment" must be str; got NoneType instead: '
"None']"
],
),
(
{"annotationDate": "2010-01-29T18:30:22Z"},
[
"Error while constructing Annotation: ['SetterError Annotation: type of "
'argument "spdx_id" must be str; got NoneType instead: None\', '
'\'SetterError Annotation: type of argument "annotation_type" must be '
"spdx_tools.spdx.model.annotation.AnnotationType; got NoneType instead: None', "
'\'SetterError Annotation: type of argument "annotator" must be '
"spdx_tools.spdx.model.actor.Actor; got NoneType instead: None', 'SetterError Annotation: "
'type of argument "annotation_comment" must be str; got NoneType instead: '
"None']"
],
),
{"annotator": "Person: Jane Doe ()"},
{"annotationDate": "2010-01-29T18:30:22Z"},
],
)
def test_parse_incomplete_annotation(incomplete_annotation_dict, expected_message):
def test_parse_incomplete_annotation(incomplete_annotation_dict):
annotation_parser = AnnotationParser()

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
annotation_parser.parse_annotation(incomplete_annotation_dict)

TestCase().assertCountEqual(err.value.get_messages(), expected_message)
10 changes: 1 addition & 9 deletions tests/spdx/parser/jsonlikedict/test_checksum_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,5 @@ def test_parse_incomplete_checksum():
checksum_parser = ChecksumParser()
checksum_dict = {"algorithm": "SHA1"}

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
checksum_parser.parse_checksum(checksum_dict)

TestCase().assertCountEqual(
err.value.get_messages(),
[
'Error while constructing Checksum: [\'SetterError Checksum: type of argument "value" must be str; '
"got NoneType instead: None']"
],
)
41 changes: 6 additions & 35 deletions tests/spdx/parser/jsonlikedict/test_creation_info_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,37 +60,18 @@ def test_parse_creation_info():


@pytest.mark.parametrize(
"incomplete_dict,expected_message",
"incomplete_dict",
[
(
{"spdxVersion": "2.3", "SPDXID": DOCUMENT_SPDX_ID, "name": "Example Document"},
["Error while parsing document Example Document: ['CreationInfo does not exist.']"],
),
(
{"creationInfo": {"created": "2019-02-01T11:30:40Z"}},
[
"Error while constructing CreationInfo: ['SetterError CreationInfo: type of "
'argument "spdx_version" must be str; got NoneType instead: None\', '
'\'SetterError CreationInfo: type of argument "spdx_id" must be str; got '
"NoneType instead: None', 'SetterError CreationInfo: type of argument "
"\"name\" must be str; got NoneType instead: None', 'SetterError "
'CreationInfo: type of argument "document_namespace" must be str; got '
"NoneType instead: None', 'SetterError CreationInfo: type of argument "
"\"creators\" must be a list; got NoneType instead: None', 'SetterError "
'CreationInfo: type of argument "data_license" must be str; got NoneType '
"instead: None']"
],
),
{"spdxVersion": "2.3", "SPDXID": DOCUMENT_SPDX_ID, "name": "Example Document"},
{"creationInfo": {"created": "2019-02-01T11:30:40Z"}},
],
)
def test_parse_incomplete_document_info(incomplete_dict, expected_message):
def test_parse_incomplete_document_info(incomplete_dict):
creation_info_parser = CreationInfoParser()

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
creation_info_parser.parse_creation_info(incomplete_dict)

TestCase().assertCountEqual(err.value.get_messages(), expected_message)


def test_parse_invalid_creation_info():
creation_info_parser = CreationInfoParser()
Expand All @@ -105,15 +86,5 @@ def test_parse_invalid_creation_info():
"dataLicense": None,
}

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
creation_info_parser.parse_creation_info(doc_dict)

TestCase().assertCountEqual(
err.value.get_messages(),
[
"Error while constructing CreationInfo: ['SetterError CreationInfo: type of "
'argument "document_namespace" must be str; got NoneType instead: None\', '
'\'SetterError CreationInfo: type of argument "data_license" must be str; got '
"NoneType instead: None']"
],
)
36 changes: 36 additions & 0 deletions tests/spdx/parser/jsonlikedict/test_error_message.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from unittest import TestCase

import pytest

from spdx_tools.spdx.parser.error import SPDXParsingError
from spdx_tools.spdx.parser.jsonlikedict.package_parser import PackageParser


# To avoid duplication we use this invalid package as a proxy for the exact comparison of the generated error message.
# For all other classes we only check that a TypeError is raised if an incorrect type is specified.
def test_error_message():
package_parser = PackageParser()
package = {
"SPDXID": "SPDXRef-Package",
"downloadLocation": 5,
"attributionTexts": ["text", 5, {"test": "data"}],
"packageFileName": 10,
}

with pytest.raises(SPDXParsingError) as err:
package_parser.parse_package(package)

TestCase().assertCountEqual(
err.value.get_messages(),
[
'Error while constructing Package: [\'SetterError Package: argument "name" '
"(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, "
"spdx_tools.spdx.model.spdx_none.SpdxNone]: 5', 'SetterError Package: "
'argument "file_name" (int) did not match any element in the union: '
"[str, NoneType]: 10', 'SetterError Package: item 1 of argument "
"\"attribution_texts\" (list) is not an instance of str: [\\'text\\', 5, "
"{\\'test\\': \\'data\\'}]']"
],
)
13 changes: 1 addition & 12 deletions tests/spdx/parser/jsonlikedict/test_extracted_licensing_info_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# SPDX-FileCopyrightText: 2022 spdx contributors
#
# SPDX-License-Identifier: Apache-2.0
from unittest import TestCase

import pytest

from spdx_tools.spdx.parser.error import SPDXParsingError
Expand Down Expand Up @@ -51,14 +49,5 @@ def test_parse_invalid_extracted_licensing_info():
"seeAlsos": ["http://people.freebsd.org/~phk/"],
}

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
extracted_licensing_info_parser.parse_extracted_licensing_info(extracted_licensing_infos_dict)

TestCase().assertCountEqual(
err.value.get_messages(),
[
"Error while constructing ExtractedLicensingInfo: ['SetterError "
'ExtractedLicensingInfo: type of argument "comment" must be one of (str, '
"NoneType); got int instead: 56']"
],
)
29 changes: 2 additions & 27 deletions tests/spdx/parser/jsonlikedict/test_file_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,22 +93,6 @@ def test_parse_file(copyright_text, expected_copyright_text):
assert file.attribution_texts == ["Some attribution text."]


def test_parse_incomplete_file():
file_parser = FileParser()
file_dict = {"SPDXID": "SPDXRef-File", "fileName": "Incomplete File"}

with pytest.raises(SPDXParsingError) as err:
file_parser.parse_file(file_dict)

TestCase().assertCountEqual(
err.value.get_messages(),
[
"Error while constructing File: ['SetterError File: type of argument "
'"checksums" must be a list; got NoneType instead: None\']'
],
)


def test_parse_invalid_files():
file_parser = FileParser()
files = [
Expand All @@ -129,20 +113,11 @@ def test_parse_invalid_files():
{"algorithm": "MD", "checksumValue": "624c1abb3664f4b35547e7c73864ad24"},
],
},
{"SPDXID": "SPDXRef-File", "fileName": "Incomplete File"},
]

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
parse_list_of_elements(files, file_parser.parse_file)
TestCase().assertCountEqual(
err.value.get_messages(),
[
"Error while constructing File: ['SetterError File: type of argument "
'"checksums" must be a list; got NoneType instead: None\']',
'Error while constructing File: [\'SetterError File: type of argument "name" '
"must be str; got NoneType instead: None']",
"Error while parsing File: [\"Error while parsing Checksum: ['Invalid ChecksumAlgorithm: MD']\"]",
],
)


def test_parse_file_types():
Expand Down
75 changes: 13 additions & 62 deletions tests/spdx/parser/jsonlikedict/test_package_parser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,53 +228,24 @@ def test_parse_package(


@pytest.mark.parametrize(
"incomplete_package_dict,expected_message",
"incomplete_package_dict",
[
(
{"SPDXID": "SPDXRef-Package"},
[
"Error while constructing Package: ['SetterError Package: type of "
"argument \"name\" must be str; got NoneType instead: None', 'SetterError Package: type of argument "
'"download_location" must be one of (str, spdx_tools.spdx.model.spdx_no_assertion.SpdxNoAssertion, '
"spdx_tools.spdx.model.spdx_none.SpdxNone); "
"got NoneType instead: None']"
],
),
(
{"SPDXID": "SPDXRef-Package", "name": 5, "downloadLocation": "NONE"},
[
"Error while constructing Package: ['SetterError Package: type of argument "
'"name" must be str; got int instead: 5\']'
],
),
{"SPDXID": "SPDXRef-Package"},
{"SPDXID": "SPDXRef-Package", "name": 5, "downloadLocation": "NONE"},
{
"SPDXID": "SPDXRef-Package",
"name": "Example Package",
"downloadLocation": "NONE",
"checksums": [{"algorithm": "SHA", "value": "1234"}],
},
],
)
def test_parse_incomplete_package(incomplete_package_dict, expected_message):
def test_parse_invalid_package(incomplete_package_dict):
package_parser = PackageParser()

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
package_parser.parse_package(incomplete_package_dict)

TestCase().assertCountEqual(err.value.get_messages(), expected_message)


def test_parse_invalid_package():
package_parser = PackageParser()
package_dict = {
"SPDXID": "SPDXRef-Package",
"name": "Example Package",
"downloadLocation": "NONE",
"checksums": [{"algorithm": "SHA", "value": "1234"}],
}

with pytest.raises(SPDXParsingError) as err:
package_parser.parse_package(package_dict)

TestCase().assertCountEqual(
err.value.get_messages(),
["Error while parsing Package: [\"Error while parsing Checksum: ['Invalid ChecksumAlgorithm: SHA']\"]"],
)


def test_parse_packages():
package_parser = PackageParser()
Expand All @@ -289,37 +260,17 @@ def test_parse_packages():
{"SPDXID": "SPDXRef-Package", "name": "Example Package", "downloadLocation": "NONE"},
]

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
parse_list_of_elements(packages_list, package_parser.parse_package)

TestCase().assertCountEqual(
err.value.get_messages(),
[
'Error while parsing Package: ["Error while parsing Checksum: ' "['Invalid ChecksumAlgorithm: SHA']\"]",
"Error while constructing Package: ['SetterError Package: type of argument "
'"name" must be str; got int instead: 5\']',
],
)


def test_parse_external_ref():
package_parser = PackageParser()
external_ref = {"referenceType": "fix"}

with pytest.raises(SPDXParsingError) as err:
with pytest.raises(SPDXParsingError):
package_parser.parse_external_ref(external_ref)

TestCase().assertCountEqual(
err.value.get_messages(),
[
"Error while constructing ExternalPackageRef: ['SetterError "
'ExternalPackageRef: type of argument "category" must be '
"spdx_tools.spdx.model.package.ExternalPackageRefCategory; got NoneType instead: None', "
'\'SetterError ExternalPackageRef: type of argument "locator" must be str; '
"got NoneType instead: None']"
],
)


def test_parse_invalid_external_package_ref_category():
package_parser = PackageParser()
Expand Down
Loading