[Issue 305] add new json parser #366

meretp · 2022-12-09T15:08:39Z

fixes #305

Signed-off-by: Meret Behrens meret.behrens@tngtech.com

armintaenzertng

Thanks for this monumental effort :)
I have some initial remarks mostly concerned with (hopefully) improving readability

src/parser/json/actor_parser.py

src/parser/logger.py

src/parser/json/json_parser.py

src/parser/json/dict_parsing_functions.py

armintaenzertng

Looks good! :)
Most comments concern the naming of things but there are a few that go a little deeper.
I haven't looked at the tests yet, but I feel like there aren't enough yet

src/model/typing/constructor_type_errors.py

src/parser/error.py

armintaenzertng · 2022-12-20T11:48:29Z

src/parser/json/actor_parser.py

+
+        elif person_match:
+            name: str = person_match.group(1).strip()
+            email: Optional[str] = ActorParser.get_email_or_none(person_match)


Suggested change

email: Optional[str] = ActorParser.get_email_or_none(person_match)

email: Optional[str] = person_match.group(4).strip() or None

This way, we can discard the get_email_or_none method

This wouldn't work. If person_match.group(4) is None (e.g. if the brackets for email are completely missing) this would lead to an AttributeError. I also thought that this could be handled in one line but didn't find a good way besides using the method.

src/parser/json/annotation_parser.py

src/parser/logger.py

src/parser/json/annotation_parser.py

src/parser/json/creation_info_parser.py

src/parser/json/package_parser.py

src/parser/json/dict_parsing_functions.py

armintaenzertng

We'll probably need more tests to catch invalid inputs (open an issue for this) but the current state will do for now, I think.

tests/parser/test_actor_parser.py

tests/parser/test_annotation_parser.py

tests/parser/test_file_parser.py

tests/parser/test_json_parser.py

tests/parser/test_package_parser.py

tests/parser/test_relationship_parser.py

armintaenzertng

Nice going, I like the breadth of the tests much better now :)
Just a few remarks left

src/parser/json/dict_parsing_functions.py

src/parser/json/annotation_parser.py

src/parser/json/dict_parsing_functions.py

armintaenzertng · 2022-12-23T09:57:08Z

src/parser/json/relationship_parser.py

+    def parse_relationships(self, relationship_dicts: List[Dict]) -> List[Relationship]:
+        logger = Logger()
+        relationship_list = []
+        for relationship_dict in relationship_dicts:
+            relationship_list = append_parsed_field_or_log_error(logger=logger, list_to_append_to=relationship_list,
+                                                                 field=relationship_dict, method_to_parse=self.parse_relationship)
+        raise_parsing_error_if_logger_has_messages(logger)
+        return relationship_list


The new double-lambda expressions may not be the prettiest (we might even consider helper methods which return the required functions there) but we lose quite a lot of weight in return, so I think it is worth it

src/parser/json/relationship_parser.py

tests/parser/test_creation_info_parser.py

tests/parser/test_package_parser.py

tests/parser/test_relationship_parser.py

armintaenzertng

Thanks for the effort! :)
Only two minor comments left from my side

armintaenzertng · 2022-12-28T13:16:12Z

src/parser/json/dict_parsing_functions.py

+
+
+def parse_field_or_no_assertion_or_none(field: Optional[str], method_for_field: Callable = lambda x: x) -> Union[
+    SpdxNoAssertion, SpdxNone, Any]:


Writing Any in a Union does not make sense (as it just overwrites the other entries)

Suggested change

SpdxNoAssertion, SpdxNone, Any]:

SpdxNoAssertion, SpdxNone, str, None]:

see also the next method below

I see, but depending on the method_for_field the return value could also be a LicenseExpressions or an Actor or anything else..So probably I should just write Any as return type?

armintaenzertng · 2022-12-28T13:17:34Z

src/parser/json/package_parser.py

+        supplier: Optional[Union[Actor, SpdxNoAssertion]] = parse_field_or_log_error(logger,
+                                                                                     package_dict.get("supplier"),
+                                                                                     lambda
+                                                                                         x: parse_field_or_no_assertion(
+                                                                                         x,
+                                                                                         self.actor_parser.parse_actor))


this one escaped your reformatting ;)

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…XParsingError if construction fails Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…eadability Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…with exception handling Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…ationships Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…s and method to handle exceptions when appending a list Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…all changes Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…ueError due to wrong format Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…_str_to_enum_name Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…use assertCountEqual to compare lists Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

…r SpdxNoAssertion Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

meretp mentioned this pull request Dec 9, 2022

WIP: [issue-305] refactor parser #319

Closed

nicoweidner force-pushed the refactor-python-tools branch from 7aa8bb9 to dbe17bb Compare December 9, 2022 22:09

meretp force-pushed the issue-305_json_parser branch 5 times, most recently from 45d6c65 to c675847 Compare December 14, 2022 07:22

meretp marked this pull request as ready for review December 14, 2022 07:23

meretp marked this pull request as draft December 14, 2022 07:24

meretp marked this pull request as ready for review December 14, 2022 07:54

meretp changed the title ~~[Issue 305] refactor json parser~~ [Issue 305] add new json parser Dec 14, 2022

armintaenzertng reviewed Dec 15, 2022

View reviewed changes

meretp requested a review from armintaenzertng December 15, 2022 14:49

armintaenzertng requested changes Dec 20, 2022

View reviewed changes

armintaenzertng requested changes Dec 21, 2022

View reviewed changes

meretp force-pushed the issue-305_json_parser branch from e024400 to 0eb0629 Compare December 21, 2022 13:11

meretp requested a review from armintaenzertng December 22, 2022 12:01

armintaenzertng reviewed Dec 23, 2022

View reviewed changes

meretp requested a review from armintaenzertng December 28, 2022 13:03

armintaenzertng approved these changes Dec 28, 2022

View reviewed changes

meretp added 10 commits December 28, 2022 15:19

[issue-305] add new parser

296816b

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, refactor] add method to construct an object and raise SPD…

cd095e2

…XParsingError if construction fails Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, refactor] annotation_parser: extract methods to improve r…

f2f91fd

…eadability Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, refactor] add methods to parse required/ optional fields …

190cd5a

…with exception handling Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, refactor] relationship_parser: extract dict to invert rel…

2834000

…ationships Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, refactor] add method to raise error if logger has message…

6297673

…s and method to handle exceptions when appending a list Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] refactor methods in dict_parsing_functions.py, sm…

4741a43

…all changes Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, refactor] json_parser

080d848

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, reformat]

5826922

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305] add testcases and update license_expression parser

e6332cb

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

meretp added 12 commits December 28, 2022 15:21

[issue-305, refactor] delete duplicated check for error type

1f6d5b6

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] fix messages, naming, type hints

fc980b1

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] refactor relationship_parser

3fe3e11

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] refactor snippet_parser

0be1780

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] make naming consistent

c5b8d3c

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] add test for dict parsing functions and catch Val…

03cce38

…ueError due to wrong format Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] add None handling for required fields

2dcd125

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] make error messages consistent, add test for json…

50c3038

…_str_to_enum_name Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] add tests, change test data, naming of tests and …

562f288

…use assertCountEqual to compare lists Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] add method to parse fields that can be SpdxNone o…

a722036

…r SpdxNoAssertion Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] refactor parse_field_or_log_error

c8851d8

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

[issue-305, review] reformat, type hints, fix typos, error messages

347051a

Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>

meretp force-pushed the issue-305_json_parser branch from c1ee766 to 347051a Compare December 28, 2022 14:21

meretp merged commit f3bca2d into spdx:refactor-python-tools Dec 28, 2022

meretp deleted the issue-305_json_parser branch December 28, 2022 14:42

This was referenced Dec 28, 2022

Refactor JSON parser #305

Closed

add cli parser for json files #388

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue 305] add new json parser #366

[Issue 305] add new json parser #366

meretp commented Dec 9, 2022

armintaenzertng left a comment

armintaenzertng left a comment

armintaenzertng Dec 20, 2022

armintaenzertng Dec 20, 2022

meretp Dec 21, 2022

armintaenzertng left a comment

armintaenzertng left a comment

armintaenzertng Dec 23, 2022

armintaenzertng left a comment

armintaenzertng Dec 28, 2022

meretp Dec 28, 2022 •

edited

armintaenzertng Dec 28, 2022

meretp Dec 28, 2022

	email: Optional[str] = ActorParser.get_email_or_none(person_match)
	email: Optional[str] = person_match.group(4).strip() or None



		def parse_field_or_no_assertion_or_none(field: Optional[str], method_for_field: Callable = lambda x: x) -> Union[
		SpdxNoAssertion, SpdxNone, Any]:

	SpdxNoAssertion, SpdxNone, Any]:
	SpdxNoAssertion, SpdxNone, str, None]:

[Issue 305] add new json parser #366

[Issue 305] add new json parser #366

Conversation

meretp commented Dec 9, 2022

armintaenzertng left a comment

Choose a reason for hiding this comment

armintaenzertng left a comment

Choose a reason for hiding this comment

armintaenzertng Dec 20, 2022

Choose a reason for hiding this comment

armintaenzertng Dec 20, 2022

Choose a reason for hiding this comment

meretp Dec 21, 2022

Choose a reason for hiding this comment

armintaenzertng left a comment

Choose a reason for hiding this comment

armintaenzertng left a comment

Choose a reason for hiding this comment

armintaenzertng Dec 23, 2022

Choose a reason for hiding this comment

armintaenzertng left a comment

Choose a reason for hiding this comment

armintaenzertng Dec 28, 2022

Choose a reason for hiding this comment

meretp Dec 28, 2022 • edited

Choose a reason for hiding this comment

armintaenzertng Dec 28, 2022

Choose a reason for hiding this comment

meretp Dec 28, 2022

Choose a reason for hiding this comment

meretp Dec 28, 2022 •

edited