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

(feat)(falcon/routing) Allow numbers in template fields #454

Conversation

BenjamenMeyer
Copy link
Contributor

Allow URI template fields to contain numbers so long as the field name does not start
with a numeral.

Example: "test123" is valid, while "123test" is not

Supports Issue #35

Note: '\w' = [0-9A-Za-z_]

Allow URI template fields to contain numbers so long as the field name does not start
with a numeral. Example: "test123" is valid, while "123test" is not
@kgriffs
Copy link
Member

kgriffs commented Apr 15, 2015

#484 should address this. Can you give it a try?

@BenjamenMeyer
Copy link
Contributor Author

@kgriffs Sorry, I just noticed your comment.

I checked out PR #484 (as requested) and actually merged master into this PR, namely so that I could just check the tests more carefully. Since he already had the negative test (

def test_field_name_cannot_start_with_digit(self):
) I did this primarily to check my positive test which passed.

The PR seems to cover my positive test in the complex test (

def test_complex(self):
), just a little more indirect. The merge basically reduces this PR to adding the couple tests more explicitly.

matches = list(re.finditer('{([-_a-zA-Z0-9]+)}', seg))
seems to do what my regex changes does, with the negative prevented by
if re.search('{\d', uri_template):
. I think the negative test has the potential to be bypassed, but if someone is doing that then the bug is in their code, not Falcon's.

In the end, what I reported looks to be fixed. I'll leave it to you whether to accept my extra couple tests.
If you want them, then merge. If not, you can close the PR. I'm happy with the result.

@kgriffs
Copy link
Member

kgriffs commented Aug 7, 2015

Merged manually. Thanks @BenjamenMeyer !

@kgriffs kgriffs closed this Aug 11, 2015
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