Strsplittrail #27
Strsplittrail #27
Conversation
Seemed to default to TRUE no matter what; also switched 'map_to_ansi' to call 'non_matching' with 'empty=TRUE' since that seemed to be what it was doing before fix
This lines up with what 'substr' does. Note we fixed this in 'col_substr' instead of in 're_table' which would have been the other option, but that could possibly have cause other stuff to break
|
Fyi, I'll submit the resolved conflicts later today. |
Conflicts: R/utils.r inst/NEWS.md
|
@gaborcsardi, fyi, the branch is now up to date. |
|
@gaborcsardi, let me know if you intend to eventually merge this one or not. Fine if you don't, but it would be helpful for me to know one way or other since it affects how I write my package that imports crayon. |
| chunks <- lapply( | ||
| chunks, | ||
| function(y) | ||
| if(nrow(y) > 1L && !tail(y, 1L)[, "length"]) head(y, -1L) else y |
There was a problem hiding this comment.
Why the nrow(y) > 1L condition? I have
❯ strsplit("", "a")[[1]]
character(0)so we want to drop a single "" as well, I think.
There was a problem hiding this comment.
I'll have to look at it again later. Clearly this is wrong, but I think I was trying to ensure that we don't drop this:
> strsplit("a", "a")[[1]]
[1] ""
Hopefully I can look at it this evening.
empty strings, empty splits, etc.
|
Okay, so unfortunately addressing the corner case you highlight is not completely straightforward. This latest push is NOT ready for merging as there remains some corner cases and I want to test this more thoroughly. I put it here for your inspection in because the (currently partial) solution I put in is not particularly elegant. As far as I can tell, I can't easily rely on the existing infrastructure to handle the corner cases, as passing 0-row chunks causes Let me know if have any concerns with me just handling the corner cases as the EDIT: actually, I'm not sure we can modify the internal funs as the issue is that we're basically doing something like |
|
Thanks! I'll try to look at it this evening. |
Throw an error if 'split' is length greater than 1 instead of letting 'gregexpr' gag on it. Explicitly coerce to character since 'gregexpr' doesn't necessarily do it. Substitute "" for zero length splits to mimic strsplit behavior.
|
Okay, this should be good to go now, assuming you're okay with how I've addressed the issues. I wish |
|
|
||
| expect_equal(col_strsplit("", "-"), list("")) |
There was a problem hiding this comment.
Note that these tests no longer behave like they used to so I changed them.
|
I addressed the last remaining issue I opened. Note that one issue is that Also, appveyor is failing, but I think the issue is not related to the code changes I made (for some reason Finally, haven't heard much one way or another on this pull request. Feedback would be appreciated. Thanks. |
|
I am sorry this is taking long. I'll look at it today. |
|
Let me know if you want me to simplify this pull request by breaking it up into smaller ones. |
|
Thanks much! I am sorry that it took so long! |
|
Great, thanks for accepting the pull. |
Proposed "fix" for #26 (Fixes #26). A few noteworthy comments:
col_strsplit. It could potentially be "fixed" incrayon:::re_tablewhich would be cleaner, but would affect multiple other functionsemptyparameter fornon_matching; since it wasn't being used (I think), I updated all calls to that function to specifyempty=TRUEto avoid changing behaviorLet me know if you want me to change / revert any of this.