Fix #24214: Extended isvalid() for SubString #24231

Vaibhavdixit02 · 2017-10-20T08:56:10Z

nalimilan · 2017-10-21T09:17:47Z

test/strings/basic.jl

    @test isvalid(String, UInt8[0xfe, 0x80, 0x80, 0x80, 0x80, 0x80]) == false
+    #issue #24214 Check valid SubString
+    @test isvalid(lstrip(" ghjki")) == true
+    @test isvalid(String(UInt8[0xfe, 0x80, 0x80, 0x80, 0x80, 0x80]), 1,2) == false


Looks like you forgot SubString(.

bkamins · 2017-10-21T11:45:51Z

SubString does not have to be valid in general as checking this would introduce too much performance overhead (and SubString should be a light wrapper around AbstractString).
The check that is performed only makes sure that the first and last index passed to SubString constructor pass isvalid test (and the indices between first and last can be anything).

bkamins · 2017-10-21T11:49:10Z

@Vaibhavdixit02 I would also to recommend to update the docstrings of isvalid in utf8proc.jl.

bkamins · 2017-10-21T14:55:08Z

base/strings/string.jl

    # 2: valid UTF-8

-isvalid(::Type{String}, s::Union{Vector{UInt8},String}) = byte_string_classify(s) != 0
+isvalid(::Type{String}, s::Union{Vector{UInt8},String,SubString{String}}) = byte_string_classify(s) != 0


@Vaibhavdixit02 I would recommend to add isvalid(<:SubString{String}) definition and update an appropriate docstring.

Why is that required? I am not familiar with any subtype of SubString{String}, are there any?

You are right - there are none. I meant that single argument isvalid should be defined as:

isvalid(s::Union{String,SubString{String}}) = isvalid(String, s)

and that this change should be also reflected in docstrings

Ah! Got it.

I meant that in current implementation it seems that without explicitly telling String even Vector{UInt8} returns false so is this extension something that would be added or is it not in line with isvalid()'s required features? @stevengj perhaps you could give your view too?

I meant that isvalid(::Vector{UInt8}) would now throw method error and since isvalid(::Type{String}, ::Vector{UInt8}) is allowed I would also allow it.

isvalid(::Vector{UInt8}) would be really confusing. There's way more things Vector{UInt8} can be other than a string.

isvalid(SubString{String}) should work though, right? @yuyichao

isvalid(String, ::Vector{UInt8}) should continue to work. I agree that isvalid(::Vector{UInt8}) makes no sense.

bkamins · 2017-10-21T14:55:48Z

base/strings/utf8proc.jl

 Returns `true` if the given value is valid for that type. Types currently can
 be either `Char` or `String`. Values for `Char` can be of type `Char` or [`UInt32`](@ref).
-Values for `String` can be of that type, or `Vector{UInt8}`.
+Values for `String` can be of that type, or `Vector{UInt8}`, or `SubString`.


@Vaibhavdixit02 This should be SubString{String} as generic strings are not supported - right?

bkamins · 2017-10-21T15:04:28Z

@Vaibhavdixit02 One more thing (I do not know why Github has eaten this comment twice):
I think that you need also to define isvalid(<:SubString{String}) (and add an appropriate docstring update). Probably using Union, as now isvalid(lstrip(" sadfs ")) will fail. Right?

Vaibhavdixit02 · 2017-10-21T15:32:51Z

It seems that I am getting a String type from lstrip in my system for some reason.

And why should isvalid(lstrip(" sadfs ")) fail? I mean the type returned by isvalid(lstrip(" sadfs")) and isvalid(lstrip(" sadfs ")) are same, right? I am not able recreate the problem in my system.

nalimilan · 2017-10-21T15:47:47Z

Better create a SubString explicitly than calling lstrip, as the implementation could change and then the test wouldn't serve its purpose anymore.

bkamins · 2017-10-21T15:53:12Z

@nalimilan agreed.

@Vaibhavdixit02 on 0.6 lstrip returns String but this has been changed in 0.7.

bkamins · 2017-10-21T22:14:24Z

base/strings/utf8proc.jl


 Returns `true` if the given value is valid for its type, which currently can be either
-`Char` or `String`.
+`Char` or `String` or `SubString` (it is considered a valid `String` type here).


or SubString{String}

I don't understand what "(it is considered a valid String type here)" means. SubString is not a String.

I agree that SubString is not a String but what I meant to say here was that here we get a true on isvalid() for SubString because we are kind of treating them as String of sorts. I guess the wording is a little misleading but just saying "Char or String or SubString{String}." would be a little inappropriate too in my opinion because the entire reason of the issue and this pull request was that SubString{String} should be treated as valid in isvalid.

There's indeed something weird with isvalid(String, ss::SubString). Maybe it should be isvalid(AbstractString, ss::SubString). Then it would be more natural for SubString to be a supported type.

Yeah I agree or else if SubString was a subtype of String it would make more sense. I wrote the docs thinking something along those lines to explain it.

The doc should be SubString{String} as I have suggested above. Additionally the code should remain String not AbstractString.

Actually the documentation it is good as:

Returns `true` if the given value is valid for its type, which currently can be either `Char` or `String` or `SubString{String}`.

Why? We are checking if s::SubString{String} is valid for its type (this is what documentation states) and it is valid if String(s) would be a valid String as it is a substing of String.

That makes sense.

Vaibhavdixit02 · 2017-10-22T17:15:59Z

I think everything is fixed now?

joa-quim · 2017-11-19T20:52:15Z

Should this PR get the 1.0 Tag? It fixes a break for code that works on 0.6

StefanKarpinski · 2017-11-20T13:35:23Z

Bug fixes are not release-blocking, but if this gets updated and passes tests, we can merge it.

joa-quim · 2017-11-20T18:55:45Z

I know this isn't a blocker issue but neither are all tagged 1.0 issues (e.g. #12902). And the test errors is something not easy to understand for me. For this PR the first error says

*** This error is usually fixed by running make clean. If the error persists, try make cleanall. ***

and looking at the PR's list, at least in the first page all PRs are marked as failing the tests.

StefanKarpinski · 2017-11-20T21:55:37Z

Not sure what's up with the tests. #12902 is on the milestone, trivial though it is, because changing a name is breaking. Milestone tags are not a priority assignment.

Vaibhavdixit02 · 2017-12-11T15:58:32Z

Hey, does it seem like this PR will be merged?

StefanKarpinski · 2017-12-11T17:40:57Z

Yes, this is good to have it just fell off my radar. Can you rebase it on top of #24999?

Vaibhavdixit02 · 2017-12-27T20:30:29Z

I have created a new PR #25285 because of the changes in #24999, I should close this PR now I think?

stevengj · 2017-12-29T18:20:25Z

(Note that in general you should do a forced push to update the existing PR rather than opening a new PR.)

Fix #24214: Extended isvalid() for SubString

247b692

KristofferC added needs tests Unit tests are required for this change strings "Strings!" labels Oct 20, 2017

Vaibhavdixit02 added 2 commits October 20, 2017 17:59

Added test #24214

19ae7e0

fix #24214

fe0abdd

nalimilan reviewed Oct 21, 2017

View reviewed changes

Fix #24214

060fbb6

Vaibhavdixit02 added 3 commits October 21, 2017 20:02

Added docs for #24214

5804758

Added docs for #24214

b7e156e

Added docs for #24214

6835cda

bkamins reviewed Oct 21, 2017

View reviewed changes

Added docs for #24214

47aa3e8

Changes and additions #24214

30454dc

bkamins reviewed Oct 21, 2017

View reviewed changes

Vaibhavdixit02 added 2 commits October 22, 2017 03:46

Changes and additions #24214

05c6d44

Change in documentation

299aa69

stevengj closed this Dec 29, 2017

Uh oh!

Fix #24214: Extended isvalid() for SubString #24231

Fix #24214: Extended isvalid() for SubString #24231

Uh oh!

Conversation

Vaibhavdixit02 commented Oct 20, 2017 • edited by stevengj Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkamins commented Oct 21, 2017

Uh oh!

bkamins commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkamins commented Oct 21, 2017

Uh oh!

Vaibhavdixit02 commented Oct 21, 2017

Uh oh!

nalimilan commented Oct 21, 2017

Uh oh!

bkamins commented Oct 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vaibhavdixit02 commented Oct 22, 2017

Uh oh!

joa-quim commented Nov 19, 2017

Uh oh!

StefanKarpinski commented Nov 20, 2017

Uh oh!

joa-quim commented Nov 20, 2017

Uh oh!

StefanKarpinski commented Nov 20, 2017

Uh oh!

Vaibhavdixit02 commented Dec 11, 2017

Uh oh!

StefanKarpinski commented Dec 11, 2017

Uh oh!

Vaibhavdixit02 commented Dec 27, 2017

Uh oh!

stevengj commented Dec 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Vaibhavdixit02 commented Oct 20, 2017 •

edited by stevengj

Loading