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

PR 16130 deprecate symbol ==> Symbol #16130

Closed
wants to merge 1 commit into from
Closed

PR 16130 deprecate symbol ==> Symbol #16130

wants to merge 1 commit into from

Conversation

jeffreysarnoff-dev
Copy link

@jeffreysarnoff-dev jeffreysarnoff-dev commented Apr 30, 2016

Note: (from @hayd) this was eventually merged in #16154 (originally #15995).

PR 16130 deprecate symbol ==> Symbol (clean version)
[apologies for needing three PR numbers to do one thing]

@jeffreysarnoff-dev jeffreysarnoff-dev changed the title PR 16038 deprecate symbol ==> Symbol PR 16130 deprecate symbol ==> Symbol Apr 30, 2016
@jeffreysarnoff-dev
Copy link
Author

jeffreysarnoff-dev commented Apr 30, 2016

corrected one problem -- force pushed the change into the single commit.
how do I get AppVeyor and Travis to recheck? never mind -- I see they are doing it

@hayd
Copy link
Member

hayd commented Apr 30, 2016

It looks like you're yet to address all @stevengj's comments from #15995. In future it's better IMO to force push to the original branch / PR. That way those comments become become "outdated diffs" once they are fixed.

@jeffreysarnoff-dev
Copy link
Author

@hayd I know you are correct. I am very unfamiliar with this process and after trying it the 'right way' umpteen times without success I was able to proceed this less than perfect way. Regarding @stevengj's comments, as there was an unexpected, indirect conflict that arose when I did attempt incorporating his comments -- one that I did not know how to fix, on the advice of others I went back to the original purpose which was to replace symbol with Symbol. Once this is clean, the checks are good and it can be accepted, then it is possible to readdress the additional suggestions in a separate PR.

@jeffreysarnoff-dev
Copy link
Author

The last change was to comment out a test in replutil.jl (done in accord with prior advice). The advice (as I understood it) was to eliminate the test involving Symbol generating: "expected failure but no error thrown" because this failure mode was no longer relevant.
[waiting for AppVeyor to see if I got the right one]

@jeffreysarnoff-dev
Copy link
Author

The test commented out is at lines 230-2 of test/replutil.jl

@jeffreysarnoff-dev
Copy link
Author

jeffreysarnoff-dev commented Apr 30, 2016

This began as #15995 and took a detour into #16308 before coming to rest here.

@test contains(err_str, "MethodError: no method matching Symbol()")
#PR 16130 (Symbol)
#err_str = @except_str Symbol() MethodError
#@test contains(err_str, "MethodError: no method matching Symbol()")
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using some other type that doesn't have a 0-argument constructor? that would capture the point of this test

Choose a reason for hiding this comment

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

@tkelman I did not realize that here Symbol() is not important as Symbol per se. I will see how this works substituting Float64(), which generates a MethodError

@nalimilan
Copy link
Member

Apart from the comment this looks good to me!

@Jeffrey-Sarnoff
Copy link

@nalimilan @tkelman The only change was to remove the comment
(both checks before that change worked).
"Build execution time has reached the maximum allowed time for your plan (120 minutes)."
Should I restart Appveyor? How do I do that?

@nalimilan
Copy link
Member

That's a random failure, next time it will likely succeed.

@Jeffrey-Sarnoff
Copy link

red-clay-for-modified-cello-ensemble-score-image
notes noted

@nalimilan
Copy link
Member

Let's merge this?

@hayd
Copy link
Member

hayd commented Apr 30, 2016

Has symbol been deprecated or removed?

@JeffreySarnoff
Copy link
Contributor

symbol has been deprecated

@jeffreysarnoff-dev
Copy link
Author

@hayd OOPS! nice catch -- that got lost in translation -- remedying now

@jeffreysarnoff-dev
Copy link
Author

@hayd symbol is once again deprecated, but now there are conflicts and this is where I have had trouble. Each time I have tried to fix a merge conflict, I have messed up my repository.
And I don't know what the conflict is; I see " Can’t automatically merge. Don’t worry, you can still create the pull request."
.

@nalimilan
Copy link
Member

Damn, there are conflicts with master again. Could you rebase?

@jeffreysarnoff-dev
Copy link
Author

@nalimilan I did rebase locally -- and I could push that up to github ... but each time I did that in the past, there was trouble. After pushing the correction to deprecated.jl, here is what I did locally:
git fetch
git rebase -i origin/master
[a git-rebase-todo file opened in an editor, it was empty and I left it so]

@jeffreysarnoff-dev
Copy link
Author

@nalimilan git says "Successfully rebased and updated refs/heads/jas/deprecate_symbol"
locally

@KristofferC
Copy link
Member

KristofferC commented Apr 30, 2016

I don't think you want to interactively rebase. Just git rebase origin/master should try to replay your PR on top of the lastest commit on the master branch. When a commit fails to merge git will yell at you and if you do git status you can see what files have a merge conflict. Fix these, add them with git add and then run git rebase --continue. Rinse and repeat.

@jeffreysarnoff-dev
Copy link
Author

@KristofferC for me "master" is https://github.com/jeffreysarnoff-dev/julia which is now 6 commits behind JuliaLang:master. my "upstream" is ..JuliaLang/julia.git. And I have no more commits, I could edit the commit message in an inconsequential way if I need to be able to force push something that is local.
What should I do to get to the point that git rebase master does what you suggest?

@jeffreysarnoff-dev
Copy link
Author

@KristofferC git status is telling me that (locally) I am on branch jas/deprecate_symbol and "Your branch is up-to-date with 'origin/jas/decprecate_symbol'

@nalimilan
Copy link
Member

git rebase origin/master should use the "real" master, not the one from your clone.

@nalimilan
Copy link
Member

@jeffreysarnoff-dev
Copy link
Author

@nalimilan I understand how git works .. I just don't like it. I have been using an app, SmartGit, along with using command line git because the app prevents me from doing some things wrong. It may set me to do other things oddly. It is that app that 'decided' origin is my github branch and upstream is JuliaLang. My local master branch points up to my github fork. Right now, I need use git on the command line to rebase things using JuliaLang master so I can see the conflicts and then address them. Any guidance is welcome -- not about how git works, about how to play this current game of cat's cradle to properly re-entangle things.

@jeffreysarnoff-dev
Copy link
Author

When I close the git-assist App, it goes away and I can do anything I want from the command line. Should I restart that app, it just picks up from whatever state the git repository is in right then.

@hayd
Copy link
Member

hayd commented Apr 30, 2016

Using upstream for JuliaLang is good, the only difference is you have to do git rebase upstream/master. You may also have to first explicitly git fetch upstream, depending on your git version/setup.

Note: The conflict is just in the deprecated.jl.

My advice is not to use a git app (or git within your IDE) until you have understood everything that the app/ide is doing at every step of the process.

@jeffreysarnoff-dev
Copy link
Author

@hayd Thank you.

@jeffreysarnoff-dev
Copy link
Author

red-clay-for-modified-cello-ensemble-score-image
above is from Red-Clay

@JeffreySarnoff
Copy link
Contributor

This has been a hike; at times I became weary and would have been uncomfortable without you:
@hayd @JeffBezanson @Keno @KristofferC @nalimilan @stevengj @timholy @tkelman @vtjnash @yuyichao (and any omitted by happenstance).

@@ -368,6 +368,9 @@ end
@deprecate flipud(A::AbstractArray) flipdim(A, 1)
@deprecate fliplr(A::AbstractArray) flipdim(A, 2)

@deprecate sub2ind{T<:Integer}(dims::Array{T}, sub::Array{T}) sub2ind(tuple(dims...), sub...)
Copy link
Member

@nalimilan nalimilan May 1, 2016

Choose a reason for hiding this comment

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

You have a lot of unrelated changes in this file.

EDIT: not a lot, just this one and sprandbool below.

@jeffreysarnoff-dev
Copy link
Author

jeffreysarnoff-dev commented May 1, 2016

I unattached the PR from the github branch -- pls see 16150 which is running the checks
if that does not do it, I will post the file of sed commands that does all the changing (which exists).

@jeffreysarnoff-dev
Copy link
Author

sweeping away more bits --

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants