Skip to content

Conversation

@rohanpm
Copy link
Contributor

@rohanpm rohanpm commented Oct 1, 2019

Matcher.regex is documented as accepting a string, but previously
if you passed an already-compiled regex, that would also be
accepted by the validator. This happened because re.compile simply
returns already-compiled regexes without an error. It would
ultimately lead to an error when executing a search, because a
compiled regex is not serializable to JSON.

Fix it to more strictly typecheck the inputs when creating the
matcher, so we fail early and with a meaningful message.

Matcher.regex is documented as accepting a string, but previously
if you passed an already-compiled regex, that would also be
accepted by the validator. This happened because re.compile simply
returns already-compiled regexes without an error. It would
ultimately lead to an error when executing a search, because a
compiled regex is not serializable to JSON.

Fix it to more strictly typecheck the inputs when creating the
matcher, so we fail early and with a meaningful message.
@rohanpm rohanpm requested a review from rajulkumar as a code owner October 1, 2019 01:02
@rohanpm
Copy link
Contributor Author

rohanpm commented Oct 1, 2019

CI is failing as the reverse-dependency test is showing that pubtools-pulp tests would fail with this change. That's expected until release-engineering/pubtools-pulp#15 is submitted. The issue is that pubtools-pulp currently relies on this missing validation for its own autotests.

So, this is fine for review now, but release-engineering/pubtools-pulp#15 must be submitted and then this should be retested before merging.

@rohanpm rohanpm merged commit 8e49181 into release-engineering:master Oct 2, 2019
@rohanpm rohanpm deleted the matcher-regex-str branch October 3, 2019 00:24
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.

5 participants