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

Support . in characters and negative numbers #213

Merged
merged 2 commits into from
Dec 7, 2017

Conversation

etiennebr
Copy link
Contributor

@etiennebr etiennebr commented Nov 23, 2017

This is in relation to #212. I don't know how is your code coverage for tests, but it seems to pass. I was expecting to break more things, especially the . in characters since it was explicitly excluded. I'd be happy to change the pull request if needed.

@CLAassistant
Copy link

CLAassistant commented Nov 23, 2017

CLA assistant check
All committers have signed the CLA.

@etiennebr etiennebr changed the title upport . in characters and negative numbers Support . in characters and negative numbers Nov 23, 2017
@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #213 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   88.18%   88.18%           
=======================================
  Files          24       24           
  Lines        1193     1193           
=======================================
  Hits         1052     1052           
  Misses        141      141
Impacted Files Coverage Δ
R/query-string.R 89.33% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c894fd...055d9bd. Read the comment docs.

* add tests
* allow `.` in string
* allow negative int
* allow negative numeric

fix rstudio#212
Copy link
Contributor

@trestletech trestletech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for contributing.

Can you add a note in the NEWS file to the effect of

* Support negative numbers in numeric path segments
* Support `.` in string path segments

R/query-string.R Outdated
re[type == "double" | type == "numeric"] <- "\\\\d*\\\\.?\\\\d*"
re <- rep("[^/]+", length(type))
re[type == "int"] <- "-?\\\\d+"
re[type == "double" | type == "numeric"] <- "-?\\\\d*\\\\.?\\\\d*"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to your code, but I believe this is actually a bug, as it matches "" as currently written. Do you mind swapping one of the \\\\d*s with a \\\\d+ so that we can be sure that there's at least one digit in the string (as we do with int)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, you're right. I'll use -?\d*\.?\d+ which would match e.g. .23, which can be interesting, I guess.

@trestletech
Copy link
Contributor

Everything looks great except that the tests/testthat/files/include/test.html seems to have been accidentally committed. If you don't mind removing that from the branch, I'll merge. Thanks again!

* don't match `.`
* don't match empty string ""
@etiennebr
Copy link
Contributor Author

Sorry about the test.html. Seems like everything is ready, but tell me if not. Thanks for the plumber package and thanks for making it so easy to contribute.

@trestletech trestletech merged commit 02fab1e into rstudio:master Dec 7, 2017
@trestletech
Copy link
Contributor

Awesome! Thanks again.

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.

4 participants