Skip to content

String: alias for new String or old UTF8String #192

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

Closed
wants to merge 1 commit into from

Conversation

StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented May 4, 2016

Closes #193.

@StefanKarpinski StefanKarpinski mentioned this pull request May 4, 2016
32 tasks
@stevengj stevengj mentioned this pull request May 4, 2016
@@ -1077,4 +1077,12 @@ if !isdefined(Base, :Threads)
export Threads
end

if isdefined(Core, :String)
Copy link
Member

Choose a reason for hiding this comment

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

Will this work on Julia 0.3?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to check VERSION >= 0.5.0-dev+3876

@StefanKarpinski
Copy link
Member Author

Updated to support 0.3 as well.

@@ -1077,4 +1077,12 @@ if !isdefined(Base, :Threads)
export Threads
end

if isdefined(Core, :String) && isdefined(Core, :AbstractString)
typealias String Core.String
export String
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why this is needed. I just tried running the latest 0.5, and String already seems to be exported.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we don't actually need it – it seems that you can import a binding from Compat that was inherited from Base.

Copy link
Member

@stevengj stevengj May 4, 2016

Choose a reason for hiding this comment

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

We don't need to import it from Compat in the first place, because you already get the symbol from Base anyway in 0.5.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the wrong binding on 0.3 and 0.4 – you want to use a thing called String that is actually Base.UTF8String.

Copy link
Member

@stevengj stevengj May 4, 2016

Choose a reason for hiding this comment

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

On 0.3 and 0.4, you export the typealias. That part is fine. What I'm saying is that on 0.5, Compat can just do nothing (no exports, no typealiases).

People should be doing using Compat to get all of the exports (if any), not import Compat: String etcetera; the latter is not how the Compat module is supposed to work.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

That won't work in this case since String is already defined and exported on both 0.3 and 0.4 – but it has a different meaning than the one we're exporting here. On 0.3, not doing the import works since at that point we just let the last export win. On 0.4, however, this will not work since we now do this:

julia> using Compat; String
WARNING: both Compat and Base export "String"; uses of it in module Main must be qualified
ERROR: UndefVarError: String not defined
 in eval(::Module, ::Any) at ./boot.jl:236

So the only way to write code that uses the name String to mean something UTF8String-like on all of 0.3, 0.4 and 0.5 is to do using Compat; import Compat.String and then use the name String. If you can think of some way around this, I'm all ears.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see.

Copy link
Contributor

Choose a reason for hiding this comment

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

can't @compat(String(...)) do a rewriting without messing with imports?

Copy link
Member

@stevengj stevengj May 5, 2016

Choose a reason for hiding this comment

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

Yes, but String types are so common that it is probably easier to do import than to sprinkle @compat all over the place.

@yuyichao
Copy link
Contributor

yuyichao commented May 8, 2016

What's the status of this?

@tkelman
Copy link
Contributor

tkelman commented May 8, 2016

still needs tests, still needs docs, and I think Stefan is traveling at the moment

yuyichao referenced this pull request in johnmyleswhite/Benchmarks.jl May 9, 2016
* Depwarn for `readall`
* Depwarn for `UTF8String`
* Workaround error when using `esc` on function argument
* Add Compat 0.7.14+ dependency
@timholy
Copy link
Member

timholy commented May 10, 2016

Bump. Would be nice to be able to start fixing packages.

@StefanKarpinski
Copy link
Member Author

All the code required to start fixing packages is merged already: all you need to do is using Compat; import Compat.String and then replace all uses of ASCIIString, UTF8String and ByteString with String. I'm speaking at conferences in Krakow this week and won't have any time until Thursday at the earliest if someone else wants to write some tests and docs.

@andreasnoack
Copy link
Member

I tried to go ahead and fix some packages following the advice in #192 (comment) but it doesn't work. Right now on 0.4 I get

julia> using Compat

julia> import Compat.String

julia> foo(x::String) = 1;

julia> foo("Hello")
ERROR: MethodError: `foo` has no method matching foo(::ASCIIString)

julia> methods(foo)
# 1 method for generic function "foo":
foo(x::UTF8String) at none:1

@yuyichao
Copy link
Contributor

Right. you can't just replace UTF8String ASCIIString and ByteString all with UTF8String on 0.4 without changing the package API.

@andreasnoack
Copy link
Member

But then I need to think. I tried to avoid that.

@stevengj
Copy link
Member

Maybe we need @compat foo::String -> foo::ByteString on 0.4?

timholy added a commit that referenced this pull request May 10, 2016
Slightly modified from #192
@timholy timholy mentioned this pull request May 10, 2016
@timholy
Copy link
Member

timholy commented May 10, 2016

Probably superseded by #197.

@timholy timholy closed this May 11, 2016
martinholters pushed a commit to martinholters/Compat.jl that referenced this pull request Jul 13, 2016
Use Base.promote_op() for arithmetic operators
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.

6 participants