implement: validate_environment_marker#220
implement: validate_environment_marker#220mr54ndm4n wants to merge 4 commits intopypa:masterfrom mr54ndm4n:validator_env_marker_parentheses
Conversation
| else: | ||
| stk.append(token[idx]) | ||
| else: | ||
| stk.append(token[idx]) |
There was a problem hiding this comment.
Can you add some comments in your parser code? I'm finding it a bit hard to follow.
| idx += 1 | ||
| if len(stk) != 1 or stk[-1] != "EXP": | ||
| problems.append("Invalid environment markers syntax") | ||
| except Exception as e: |
There was a problem hiding this comment.
I don't like this structure - it could easily obscure a bug in the code by catching an overly general exception, and it means all the interesting code is indented a level more than it needs to be.
If you want to jump out of parsing as soon as a problem is detected, I think it's OK to use an early return.
| assert len(res) == 1 | ||
| assert res[0].startswith("Invalid expression") | ||
|
|
||
| res = vem("""()))))()extra == "test"(((((((""") # No chained comparisons |
There was a problem hiding this comment.
The comment here is misleading, I think
| res = vem("""extra == "test" and or (os_name == "nt" or python_version == "2.7")""") # No chained comparisons | ||
| assert len(res) == 1 | ||
| assert res[0] == "Invalid expression \"and or\"" | ||
|
|
There was a problem hiding this comment.
Can we add a test for an empty pair of parentheses, like extra == "test" and ()?
| $""", re.IGNORECASE | re.VERBOSE) | ||
| MARKER_OP = re.compile(r'(~=|===?|!=|<=?|>=?|\s+in\s+|\s+not in\s+)') | ||
|
|
||
|
|
There was a problem hiding this comment.
Not a big deal, but as a general rule, please try not to reformat code beyond the bit that you're working on.
| VERSION_SPEC = re.compile(r'(~=|===?|!=|<=?|>=?)\s*[A-Z0-9\-_.*+!]+$', re.IGNORECASE) | ||
| REQUIREMENT = re.compile(NAME.pattern[:-1] + # Trim '$' | ||
| r"""\s*(?P<extras>\[.*\])? | ||
| r"""\s*(?P<extras>\[.*\])? |
There was a problem hiding this comment.
Please put the alignment here back how it was, it's like that deliberately.
|
I'm closing this PR and use new PR instead |
2 participants
resolve #213