Skip to content

Parse expressions with nested license names and spaces#31

Merged
pombredanne merged 9 commits intomasterfrom
29-parse-spaces
Aug 6, 2018
Merged

Parse expressions with nested license names and spaces#31
pombredanne merged 9 commits intomasterfrom
29-parse-spaces

Conversation

@pombredanne
Copy link
Member

This is a fix for the #29 bug

This is just the tests for now, to support alternative implementation

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
These can include spaces or not.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne pombredanne requested a review from tdruez August 3, 2018 19:26
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Cleanup Licensing.tokenize()

Also refactor code to move tokenizing stages in their own
functions

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Copy link
Member Author

@tdruez this should be fixing #29 and still supports parsing license symbols with spaces (such as names, etc) ass before. Your review is welcomed!

@pombredanne
Copy link
Member Author

Note that the changes are quite extensive because of some renaming for clarity.
The key changes are:

  1. Licensing.tokenize accepts a "simple" argument to use either a simple or and advanced tokenizer explicitly (this was implied before)

  2. The automaton-based tokenizer that can handle spaces and other license symbols weirdness now works using sequence of words (as separated by spaces or parens) as opposed to use characters sequences before. This also to handle the ParseError when symbol contained in exception string that is not in the Licensing #29 issues of a license name containing another license name properly (when one of the two is not for a known symbol)

  3. More tests have been added for various corner cases and parsing has been improved

As a side note, the travis config has been fixed such that tests now run OK on all OSes and Python versions

Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

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

@pombredanne the changes looks good. Thanks for all the extra unit tests.
My only concern is about the if TRACE: placed a bit everywhere.
This seems to duplicate the logic of the logging setup module. Minor in any cases.

@pombredanne
Copy link
Member Author

@tdruez in reply to:

My only concern is about the if TRACE: placed a bit everywhere.
This seems to duplicate the logic of the logging setup module. Minor in any cases.

We do not need this tracing anymore ... so let me remove this.
The reason why if commonly use a TRACE flag is that in some cases collecting the arguments to a debug logger call is more expensive than calling the logger itself... hence the flag

Also update boolean.py to 3.6

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Copy link
Member Author

@tdruez in reply to:

My only concern is about the if TRACE: placed a bit everywhere.
This seems to duplicate the logic of the logging setup module. Minor in any cases.

We do not need this tracing anymore ... so let me remove this.
The reason why if commonly use a TRACE flag is that in some cases collecting the arguments to a debug logger call is more expensive than calling the logger itself... hence the flag

I pushed a version without this and will merge shortly

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne pombredanne merged commit 7ffab9d into master Aug 6, 2018
@pombredanne pombredanne deleted the 29-parse-spaces branch August 6, 2018 14:42
pombredanne pushed a commit that referenced this pull request May 10, 2022
Signed-off-by: Jono Yang <jyang@nexb.com>
pombredanne pushed a commit that referenced this pull request May 10, 2022
Check for deps in local thirdparty directory #31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants