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

Jump to bottom

Issue 382 tag value parser #502

Merged

meretp merged 43 commits into spdx:refactor-python-tools from meretp:issue-382-tag-value-parser

Mar 9, 2023

Collaborator

meretp commented Mar 3, 2023

fixes #382

armintaenzertng requested changes

View reviewed changes

Collaborator

armintaenzertng left a comment

Nice job on yet another monumental undertaking! :)
The overall structure looks very good, but I've got a few suggestions that might enhance the code even further.
Due to the non-standard nature of the library, I even suggest including some inline comments here and there to improve readability.

tests/spdx/parser/tagvalue/test_annotation_parser.py Outdated Show resolved Hide resolved

tests/spdx/parser/tagvalue/test_creation_info_parser.py Outdated Show resolved Hide resolved

tests/spdx/parser/tagvalue/test_creation_info_parser.py Outdated Show resolved Hide resolved

tests/spdx/parser/tagvalue/test_creation_info_parser.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/lexer/tagvalue.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/parser/tagvalue.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/parser/tagvalue.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/parser/tagvalue.py Outdated

+                  @grammar_rule("pkg_comment : PKG_COMMENT text_or_line")
+                  def p_pkg_comment(self, p):
+                      self.check_that_current_element_matches_class_for_value(Package, p.lineno(1))
+                      set_value(p, self.current_element, argument_name="comment")

Collaborator

armintaenzertng Mar 7, 2023

Most of these rules are only separate from each other because the argument_name has to be set differently.
So here is a major refactoring suggestion: Put all those into p_generic_package_value and do the determination of argument_name there. Then, these last two lines above need only appear once.

Collaborator Author

meretp Mar 8, 2023

done

src/spdx/parser/tagvalue/parser/tagvalue.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/parser/tagvalue.py Outdated Show resolved Hide resolved

meretp force-pushed the issue-382-tag-value-parser branch 6 times, most recently from 5bdde3d to dcae948 Compare

March 8, 2023 15:02

armintaenzertng reviewed

View reviewed changes

src/spdx/parser/tagvalue/lexer.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/lexer/tagvalue.py Outdated

+                      t.value = t.value[1:].strip()
+                      return t
+                  @TOKEN(r"\s*((ht|f)tps?:\/\/\S*)")

Collaborator

armintaenzertng Mar 8, 2023

I would probably go with the latter: Just read in whatever "URI" is provided, and let validation handle the rest.

armintaenzertng reviewed

View reviewed changes

src/spdx/parser/tagvalue/parser.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/lexer.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/lexer/tagvalue.py Outdated Show resolved Hide resolved

armintaenzertng reviewed

View reviewed changes

Collaborator

armintaenzertng left a comment

Thanks, this starts to look really good now, probably I spent too much time with it already :D

src/spdx/parser/tagvalue/helper_methods.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/parser.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/parser.py Outdated Show resolved Hide resolved

src/spdx/parser/tagvalue/parser.py Show resolved Hide resolved

src/spdx/parser/tagvalue/parser.py Outdated Show resolved Hide resolved

armintaenzertng approved these changes

View reviewed changes

Collaborator

armintaenzertng left a comment

Thank you very much!
I think this calls for a celebration! :D

meretp added 19 commits

March 9, 2023 11:58


          [issue-382] add lexer

fdf41d3

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


          [issue-382] add tag-value parser

4e5ec68

The code is taken from the current implementation, I added a decorator function to use instead of docstrings and adapted the code according to the new data model.

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


          [refactor] use pytest fixture instead of class

8a6c45b

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


          [issue-382] implement error handling

1efdade

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


          [issue-382] catch exception if not all required arguments are provide…

71689b9

…d for construction

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


          [refactor] tests

77de34b

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


          [issue-382] add test and specify error message

7b96706

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


          [issue-382] refactor relationship_parser

589466e

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


          [issue-382] use individual logger for current_element

7bf3cb8

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


          [issue-382] add helper method to start a new current element

4d48b6e

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


          [issue-382] add test to parse whole document

5cf98f8

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


          [issue-382] add function to parse_checksum

f26aac0

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


          [issue-382] add error handling and tests for creation_info_parser

cbbeb2b

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


          [issue-382] add contains relationships

815e779

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


          [issue-382] fix example

c486af0

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


          [refactor] build parser when initializing class

1975cbf

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


          [issue-382] add tag-value parser to cli tool

0fce282

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


          [fix] raise error if name is an empty string in actor_parser

d56aedc

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


          [issue-382] check that current element matches class of parsed value

dc4b2be

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

meretp added 24 commits

March 9, 2023 11:58


          [issue-382] add negative test for annotation_parser

ae635c3

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


          [issue-382] add negative tests for extracted_licensing_info

d186f48

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


          [issue-382] rename parsing methods

d89a2c8

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


          [refactor] merge parsing functions that return the same values

0443b84

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


          [refactor] creation_info_parser

e348289

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


          [refactor] use helper method to set unique values

3d77094

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


          [refactor] merge parsing methods for package dates

1b8d9a1

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


          [issue-382] rename tests

5b62711

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


          [issue-382] add negative tests for snippet_parser

c4d1ed5

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


          [issue-382] add negative tests for package parser

d545654

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


          [issue-382] merge parsing methods

69748da

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


          [issue-382] add tests for contains relationship

71a16d6

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


          [issue-382] reformat

11d6d41

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


          [issue-382] use tag-value parser to test tag-value writer

66399f8

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


          [issue-382] fix logging

3fbe4d3

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


          [issue-382] delete element_stack as it is not needed

1a6f744

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


          [issue-382] add line number to error messages

6d06e60

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


          squashed review commits concerning test improvement

23c558f

[review] assert that only one relationship exists
[review] improve tests for package parser
[review] don't use assertCountEqual if list contains only one element
[review] add newline to <text> field
[review] delete default values in datetime
[fix] soften comparison of error messages to also support older python versions

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


          [review] fix type hint, parse_checksum and add test for the latter

1a1ca11

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


          squashed review commits concerning the tag value parser

2c77716

[review] add comments to parser to improve code readability
[review] merge parsing methods for byte_range and line_range
[review] delete superfluous except block
[review] delete superfluous call to setdefault
[review] delete superfluous case distinction
[review] rename parameter
[review] get rid of docstrings
[review] rename

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


          squashed review commits concerning structure of the tag value parser

451200d

[review] use strings instead of p.slice
[review] merge generic parsing methods
[review] parse value only if the current_element matches
[review] merge parsing methods
[review] merge error methods for current elements
[review] delete tokens for enum values and let the parser take care of correct values

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


          squashed review commits concerning structure, double quotes and naming

c972177

[review] use double quotes
[review] change file structure of tag value parser
[review] rename

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


          squashed review commits with name fixes and comment improvement

92379a6

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


          squashed review commits

97a8de4

[review] fix parsing of external document ref
[review] use only one dictionary
[review] return if multiple values for snippet range found

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

meretp force-pushed the issue-382-tag-value-parser branch from aa1ded4 to 97a8de4 Compare

March 9, 2023 11:05

meretp merged commit e5af2eb into spdx:refactor-python-tools

meretp deleted the issue-382-tag-value-parser branch

March 9, 2023 11:13

meretp mentioned this pull request

Refactor tag/value parser for the new data model #382

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet