fix: Support 'or' operators in marker expressions #82
Conversation
|
|
|
I've added tests for TBH I don't think the code is good enough for what we are trying to support here. Trying to interpret a regular grammar like this with regular expressions will only work in a narrow subset of all possible cases. Please do consider rewriting this code and implement a proper parser which could in the end support the majority of use cases and possible most environment markers (e.g. |
| @@ -9,6 +9,7 @@ | |||
| "main": "dist/index.js", | |||
| "scripts": { | |||
| "build": "tsc", | |||
| "watch": "tsc -w", | |||
There was a problem hiding this comment.
Not really related to the PR but I think this might be useful.
| @@ -20,7 +20,7 @@ | |||
| raise ImportError( | |||
| "Could not import pkg_resources; please install setuptools or pip.") | |||
|
|
|||
| PYTHON_MARKER_REGEX = re.compile(r'python_version\s*(?P<operator>==|<=|=>|>|<)\s*\'(?P<python_version>.+)\'') | |||
| PYTHON_MARKER_REGEX = re.compile(r'python_version\s*(?P<operator>==|<=|=>|>|<)\s*[\'"](?P<python_version>.+?)[\'"]') | |||
There was a problem hiding this comment.
Supporting double quotes also for python markers and making the python_version group match non-greedy to avoid blowing up by attempting to convert 6\' or python_version >= \'2 into a number.
| @@ -123,7 +123,7 @@ def create_package_as_root(package, dir_as_root): | |||
| dir_as_root[DEPENDENCIES][package_as_root[NAME]] = package_tree | |||
| return dir_as_root | |||
|
|
|||
| def satisfies_python_requirement(parsed_operator, py_version_str, sys=sys): | |||
There was a problem hiding this comment.
It's bad practice to pass in python modules as parameters. Better use mock for testing - keeps the shipped code clean and allows you to use proper contexts.
pysrc/test_pip_resolve.py
Outdated
| from collections import namedtuple | ||
|
|
||
| import unittest | ||
|
|
||
| fake_sys = namedtuple('Sys', ['version_info']) | ||
| from unittest.mock import patch |
There was a problem hiding this comment.
I assume we can use python > 3.3 for the tests. For environment testing we should really use mock if at all possible.
|
|
||
| # Gloss over the 'and' case and return true on the first matching python version | ||
|
|
||
| for sub_exp in re.split("\s*(?:and|or)\s*", cond_text): |
There was a problem hiding this comment.
We basically split up the expressions by or and and and return positive on the first matching python version.
b8fd66c to
c480f44
Compare
c480f44 to
b3164cc
Compare
| @@ -1,2 +1,3 @@ | |||
| pipenv | |||
| virtualenv | |||
| mock | |||
There was a problem hiding this comment.
Adding mock would be really useful for tests
There was a problem hiding this comment.
Thank you very much for this fix. I agree we should probably take different and more general approach but I'm happy to approve this as a quick fix in order to improve the customer experience. Thank you for all the comments and very descriptive tests, it was very helpful.
|
🎉 This PR is included in version 1.13.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This is a triage fix for Zendesk #1672 and #1817. Supporting Environment Markers with expressions like: