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
Fix bug in word() #282
Fix bug in word() #282
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber)
.
tests/testthat/test-word.r
Outdated
@@ -10,3 +10,9 @@ test_that("word extraction", { | |||
test_that("words past end return NA", { | |||
expect_equal(word("a b c", 4), NA_character_) | |||
}) | |||
|
|||
test_that("negative parameters", { | |||
expect_equal("moon",word("walk the moon", -1, -1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add the missing space before word()
?
Also you only need the first line here as the other two test the same code path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Although I will suggest to keep the other two lines since they test a specific behavior: return all the sentence if the negative start parameter is greater than the number of words. Important to have a consistent API with str_sub
.
Let me know if you want to remove it anyways!
Made all the changes. Let me know if anything else is needed! |
Thanks! |
Fixing unconsistent behavior described in #245 and adding some test.