#16038 deprecate symbol ==> Symbol #16038
Conversation
|
replaces pull request #15995, wait for the second commit here. |
|
Second commit complete, more likely than not: it is good to go. |
|
Thanks, looks good (though you could have changed all occurrences of |
|
(In general, it is better to do a forced push rather than opening a new PR, so that all the discussion goes into a single PR.) |
|
@stevengj with practice [till then, the link exists] |
| ccall(:jl_symbol_n, Ref{Symbol}, (Ptr{UInt8}, Int32), a, length(a)) | ||
| symbol(x...) = symbol(string(x...)) | ||
| Symbol(x...) = Symbol(string(x...)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
yes, this code replaces that test (which was testing that adding methods to Symbol worked correctly)
There was a problem hiding this comment.
So, there is nothing for me to do? (he asks hopefully)
There was a problem hiding this comment.
@vtjnash can confirm whether that's what he meant, but at the minimum you should change the test I referred to. For example, replace Symbol with sum() (be sure to change the error message check too).
Then, please squash the changes into 2 commits. You can do that using git rebase -i master, reordering the commits as needed, and then writing squash for those you want to merge with the preceding commit. Then, do git push jeffreysarnoff-dev +jas/deprecate_symbol to update the PR.
|
There was one fixup missed in threadcall.jl. That is fixed, it may be a separate commit. The only case of [2:end] that I did not change should not be changed afaik. |
| @@ -229,7 +229,7 @@ GeneralPeriod = Union{Period,CompoundPeriod} | |||
| (+){P<:GeneralPeriod}(x::StridedArray{P}) = x | |||
|
|
|||
| for op in (:.+, :.-) | |||
| op_ = symbol(string(op)[2:end]) | |||
| op_ = Symbol(string(op)[2]) | |||
There was a problem hiding this comment.
I don't think this is right
There was a problem hiding this comment.
You are correct, the edit is undone.
|
Is there anything I can to do to make this ready? If the stuff in the second commit reveals other issues, should I adjust this so that only the first commit is present, leaving the second commit to some other PR? |
|
You'll want to merge the last three commits (via You'll also need to rebase against master to fix the conflicts. And finally, the tests are failing with |
|
@stevengj I don't know how to do that. I know how to change this so that it only does symbol(x...) ==> Symbol. And I know how to say "I don't know how to do that." |
|
See e.g. the instructions here: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request |
|
trying anyway |
|
See also my detailed instructions at #16038 (comment). But note that the fix for the failing tests will have to go into the first commit, so that it doesn't break the build. |
unless you opt in to setting JULIA_NUM_THREADS closes #15743
|
I may have "You'll want to merge the last three commits (via git rebase -i HEAD~4 followed by a forced push) so that you only have two commits: one for the deprecation and one for the cleanup. You'll also need to rebase against master to fix the conflicts." In the event that that went as intended, and the fix is to eliminate the test (or, just avoid the conflict), How do I put that into the first commit? |
|
Commit the change, do Note your last push removed your commits and added unrelated ones... |
|
Thank you for the help -- whatever I did did not make the repository as it should be, |
|
If you can make this PR pass the tests with only the first part, that's OK. Then you'll be able to open a new PR for the improvements. |
|
updated to a single commit just replacing symbol with Symbol in .jl, .md, .rst |
|
replaced by clean version PR 16130 |
deprecate symbol ==> Symbol (first commit)
minor fixups relating to symbol(string("..")) (second commit)