-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Deprecate symbol (to Symbol) #16154
Conversation
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah damn, this'll probably fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also best not to at-notify users in commit messages, otherwise every time this gets rebased they will get a notification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, will change the message when I rebase. Er, why is this "outdated" already?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
@@ -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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this actually can be Symbol(string(op)[2])
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not if we add any other operators to the for loop
b793476
to
df832b7
Compare
From comments by stevengj in JuliaLang#15995.
@@ -144,13 +144,13 @@ representations together: | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this was here before, so let's patch that up after :)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong PR #
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, that's one of the numbers... we should use the original.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks!
@@ -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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should really not be combined with renaming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
this caused several method overwrite warnings |
While you're at it, would you be willing to add the new |
Yes, this urgently needs Compat definitions. |
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...