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

Deprecate symbol (to Symbol) #16154

Merged
merged 3 commits into from May 2, 2016

Conversation

7 participants
@hayd
Copy link
Member

commented May 1, 2016

Originally #15995 (and #16130 #16037 #16038 #16150).

In the second commit I address stevengj's comments from the original PR. Hopefully it'll pass the tests...

@@ -489,14 +489,14 @@ end
for T in indexes.parameters
T <: CartesianIndex ? (M += length(T)) : (M += 1)
end
index_length_expr = index <: Colon ? Symbol(string("Istride_", N+1)) : :(length(index))
index_length_expr = index <: Colon ? Symbol("Istride_", N+1) : :(length(index))

This comment has been minimized.

Copy link
@hayd

hayd May 1, 2016

Author Member

Ah damn, this'll probably fail.

This comment has been minimized.

Copy link
@tkelman

tkelman May 1, 2016

Contributor

also best not to at-notify users in commit messages, otherwise every time this gets rebased they will get a notification

This comment has been minimized.

Copy link
@hayd

hayd May 1, 2016

Author Member

Good to know, will change the message when I rebase. Er, why is this "outdated" already?!

This comment has been minimized.

Copy link
@hayd

hayd May 1, 2016

Author Member

I take it back, the above should work. Do you want me to amend the commit message (to remove the mistyped stevengj's at handle)?

@hayd hayd force-pushed the hayd:pr16130 branch from 360883f to 01c36d7 May 1, 2016

@@ -67,7 +67,7 @@ end
(-)(y::Period,x::TimeType) = x - y

for op in (:.+, :.-)
op_ = symbol(string(op)[2:end])
op_ = Symbol(string(op)[2:end])

This comment has been minimized.

Copy link
@hayd

hayd May 1, 2016

Author Member

I think this actually can be Symbol(string(op)[2]).

This comment has been minimized.

Copy link
@tkelman

tkelman May 2, 2016

Contributor

not if we add any other operators to the for loop

@hayd hayd force-pushed the hayd:pr16130 branch 2 times, most recently from b793476 to df832b7 May 1, 2016

Clean up Symbol usage (after symbol deprecation).
From comments by stevengj in #15995.

@hayd hayd force-pushed the hayd:pr16130 branch from df832b7 to a9df7e9 May 1, 2016

@hayd hayd changed the title Deprecate Symbol Deprecate symbol (to Symbol) May 1, 2016

@@ -144,13 +144,13 @@ representations together:

This comment has been minimized.

Copy link
@jdlangs

jdlangs May 1, 2016

Contributor

Missed the rename of :func:symbol`` here. It might also be more intuitive to rewrite it and call it the Symbol constructor rather than a regular function.

This comment has been minimized.

Copy link
@hayd

hayd May 2, 2016

Author Member

I don't think this was here before, so let's patch that up after :)

@hayd

This comment has been minimized.

Copy link
Member Author

commented May 2, 2016

Since this has already had several rounds of comments I'm going to merge this.

Thanks @jeffreysarnoff-dev / @JeffreySarnoff !

@@ -1082,6 +1082,9 @@ end
@deprecate sprand{T}(n::Integer, density::AbstractFloat, ::Type{T}) sprand(T, n, density)
@deprecate sprand{T}(r::AbstractRNG, n::Integer, density::AbstractFloat, ::Type{T}) sprand(r, T, n, density)

#16130

This comment has been minimized.

Copy link
@stevengj

stevengj May 2, 2016

Member

wrong PR #

This comment has been minimized.

Copy link
@hayd

hayd May 2, 2016

Author Member

ha, that's one of the numbers... we should use the original.

This comment has been minimized.

Copy link
@hayd

hayd May 2, 2016

Author Member

fixed, thanks!

@hayd hayd merged commit 5c4058e into JuliaLang:master May 2, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@hayd hayd deleted the hayd:pr16130 branch May 2, 2016

@@ -67,7 +67,7 @@ end
(-)(y::Period,x::TimeType) = x - y

for op in (:.+, :.-)
op_ = symbol(string(op)[2:end])
op_ = Symbol(string(op)[2])

This comment has been minimized.

Copy link
@tkelman

tkelman May 2, 2016

Contributor

this should really not be combined with renaming

This comment has been minimized.

Copy link
@hayd

hayd May 2, 2016

Author Member

It's in the second, cleanup, commit.

Edit: That is, the right hand side of this line diff is in the second commit (removing :end). The symbol -> Symbol is in the first commit.

This comment has been minimized.

Copy link
@tkelman

tkelman May 2, 2016

Contributor

there are multi character dot operators that this loop may want to be extended to, I think this change is unnecessary and could make this bug prone later

@tkelman

This comment has been minimized.

Copy link
Contributor

commented May 2, 2016

this caused several method overwrite warnings

@nalimilan

This comment has been minimized.

Copy link
Contributor

commented May 2, 2016

While you're at it, would you be willing to add the new Symbol methods (i.e. those which were previously symbol) to Compat.jl?

@Keno

This comment has been minimized.

Copy link
Member

commented May 2, 2016

Yes, this urgently needs Compat definitions.

@nalimilan nalimilan referenced this pull request May 9, 2016

Closed

Stringapalooza #16107

26 of 32 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.