Skip to content
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.

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

Fixed: change typing on register_pattern #57

Merged
merged 4 commits into from
Feb 24, 2022

Conversation

ahankinson
Copy link
Contributor

@ahankinson ahankinson commented Jan 26, 2022

Previously the register_pattern method had a type declaration of Pattern for the pattern argument, but the instance checker would raise an error if the pattern was not a string.

This commit changes the type declaration to accept str rather than Pattern. Changing the type annotation was chosen as it was the least disruptive to the current behaviour of the library.

A small test was added to check that this function does not accept compiled regex statements.

Fixes #56

Previously the `register_pattern` method had a type declaration of `Pattern` for the `pattern` argument, but the instance checker would raise an error if the pattern was not a string.

This commit changes the type declaration to accept `str` rather than Pattern. Changing the type annotation was chosen as it was the least disruptive to the current behaviour of the library.

A small test was added to check that this function does not accept compiled regex statements.

Fixes sanic-org#59
The automatic checks indicated that this import was no longer used. Also, the docstring needed to be updated.
@ahankinson
Copy link
Contributor Author

@ahopkins just checking to see if this PR needs anything further?

@ahopkins
Copy link
Member

I think ideally we should accept either a pattern or a string. Do you think you could make that change here?

Now allows both Pattern and str as arguments for the pattern.
Adjusts the error messages and documentation for the type.
Adjusts the tests to verify that Patterns are accepted as valid
arguments.

Fixes sanic-org#56
@ahankinson
Copy link
Contributor Author

OK, I have made that change. Let me know if it needs anything else!

@ahopkins
Copy link
Member

Thanks @ahankinson. I made a small change to run black and compile the pattern if it is a str.

@ahopkins ahopkins merged commit 03c0045 into sanic-org:main Feb 24, 2022
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.

Incorrect handling of pattern argument for register_pattern
2 participants