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

Overload ncodeunits on 0.7 #114

Merged
merged 6 commits into from
Feb 11, 2018
Merged

Overload ncodeunits on 0.7 #114

merged 6 commits into from
Feb 11, 2018

Conversation

ararslan
Copy link
Member

No description provided.

@ararslan ararslan requested a review from nalimilan December 16, 2017 21:42
@nalimilan
Copy link
Member

Probably worth retagging 0.3.3 after merging this.

@ararslan
Copy link
Member Author

The string stuff in this package is still woefully broken after JuliaLang/julia#24999.

@nalimilan
Copy link
Member

The stack overflow happens when filling the array to return:

    using  Unicode
    pool = CategoricalPool(["", "caf"])
    v2 = CategoricalArrays.catvalue(2, pool)
    g = graphemes(v2)
    x = Base._similar_for(1:1, eltype(g), g, Base.iteratorsize(g))
    x[1] = first(g)

So the problem happens when trying to convert a SubString{CategoricalString{UInt32}} to its own type:

convert(SubString{CategoricalString{UInt32}}, first(g))

That's really weird since we're supposed to have no-op convert methods, but the SubString method appears to take precedence.

Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Woops, I completely forgot this. I think we also need tests for ncodeunits.

@test_throws BoundsError ind2chr(v1, 0)
@test ind2chr(v2, 4) === 4
if VERSION < v"0.7.0-DEV.2949"
@test_throws BoundsError ind2chr(v1, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be converted to length calls?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there Compat for multi-arg length?

@test collect(graphemes(v2)) == collect(graphemes("café"))
if VERSION < v"0.7.0-DEV.2949"
# FIXME: This causes a StackOverflowError after the string overhaul
@test collect(graphemes(v2)) == collect(graphemes("café"))
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather keep this or mark it as @test_broken, else we'll forget about it.

src/value.jl Outdated
Base.nextind(x::CategoricalString, i::Integer) = nextind(get(x), i)
Base.prevind(x::CategoricalString, i::Integer) = prevind(get(x), i)
Base.nextind(x::CategoricalString, i::Integer) = nextind(x, Int(i))
Base.nextind(x::CategoricalString, i::Int) = nextind(get(x), i)
Copy link
Member

Choose a reason for hiding this comment

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

No need to define a method for ::Int, the conversion used in the ::Integer method will be a no-op anyway.

@ararslan
Copy link
Member Author

ararslan commented Feb 6, 2018

If you know what needs to change here, do feel free to push directly to this branch, @nalimilan. Otherwise I'll see if I can get to this a little later.

@nalimilan
Copy link
Member

I've added more changes to fix all errors and deprecations in 08_string.jl. I've stopped using "é" in the tests since it triggers failures in Base which should be handled separately.

@nalimilan nalimilan merged commit 4972555 into master Feb 11, 2018
@nalimilan nalimilan deleted the aa/strings branch February 11, 2018 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants