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

added print_range for Range objects, to show the elements of the Rang… #13615

Merged
merged 1 commit into from
Oct 19, 2015

Conversation

artkuo
Copy link
Contributor

@artkuo artkuo commented Oct 14, 2015

This is the result of a user's forum discussion about ranges. People were unhappy that 0:5 or linspace(1,10,7) does not return a vector, as they wanted. I believe it is sensible for these to be range objects (rather than being converted via collect), but the REPL display could be modified to show what the range actually looks like. The user probably wants to see some confirmation of what they were thinking, and seeing the definition doesn't help.

The proposed solution is to give REPL output like this:

julia> 0:4
5-element UnitRange{Int64}:
 0,1,2,3,4

julia> 0:100
101-element UnitRange{Int64}:
 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,…,89,90,91,92,93,94,95,96,97,98,99,100

julia> linspace(0,2.5,4)
4-element LinSpace{Float64}:
 0.0,0.833333,1.66667,2.5

This is accomplished by overloading writemime (replutil.jl) to respond to range objects, adding a print_range method (range.jl) and appropriate unit tests (test/ranges.jl). This affects display(), but the user can still use show(1:5) to return the definition 1:5. The print_range method is based on the existing print_matrix_row, and so it uses knowledge of screen size to make everything fit in one row, with horizontal dots (ellipsis) where appropriate.

Additional edits were made to show.jl, where I added a bunch of comments. I had to learn how print_matrix works, and the comments are meant to save time for someone like me, as the code is a bit opaque. There were absolutely no changes to execution of show.jl.

A few notes. I decided not to add brackets around the range. Perhaps square brackets would be appropriate, but I felt they were not needed, as a range is not exactly an array. Also, the range is printed as a single row to save vertical screen space. I thought this was acceptable because tuples are also printed across, despite acting somewhat like 1-d arrays, which are kind of "vertical." I would appreciate feedback if there are disagreements about these choices, or about the whole idea of modifying the REPL.

Take 1 was #13524
Take 2 was #13534 Fixed test to allow for 32- or 64-bit systems; changed pull request to base from master. Had a problem with docstrings due to a bug elsewhere #12737. Got stuck with git.
This is take 3: Waited for fix #13602, restored docstring to intended version, verified that everything works now and passes tests.

@artkuo artkuo mentioned this pull request Oct 14, 2015
@stevengj
Copy link
Member

You'll need to:

git checkout master
git pull origin master
git checkout new_range_display3
git rebase master
....fix merge conflicts, git add the files, then git rebase --continue....
git push -f artkuo new_range_display3

to rebase, fix the conflicts, and force-push the result back to this PR.

@artkuo artkuo force-pushed the new_range_display3 branch from 2b95853 to f5be2e6 Compare October 14, 2015 21:56
@artkuo
Copy link
Contributor Author

artkuo commented Oct 14, 2015

I guess my timing wasn't good, because that conflict arose between the time I fetched from upstream and pushed the new pull request. In any case, I did as you suggested and hopefully there are no further issues. Built and tested on my machine, everything passed. Thanks for your guidance.

@artkuo
Copy link
Contributor Author

artkuo commented Oct 16, 2015

I'm at a loss here. This same pull request had passed all tests in an earlier reincarnation #13534. Now it passes on two architectures and fails on x86_64 Linux. All the error messages seem entirely unrelated to the content of this pull request, which is also independent from other code/packages. I also don't see evidence that this is out-of-memory at Travis, so there doesn't seem an obvious way forward.

@nalimilan
Copy link
Member

I've restarted the build to see whether the failure goes away. Unfortunately, it looks like it's not possible to access the old logs after that, as the URL is the same... :-/

@nalimilan
Copy link
Member

@artkuo Looks like Travis is blocked, saying that a newer build is present for this PR. I guess you should force-push the same commits to fix it.

…e in REPL. Previous commits squashed, docstring moved back to front, builds and passes tests
@artkuo artkuo force-pushed the new_range_display3 branch from 10f7794 to 4649409 Compare October 18, 2015 18:06
@artkuo
Copy link
Contributor Author

artkuo commented Oct 18, 2015

Hokay, everything has been rebased off latest upstream master, everything squashed, and force-pushed. This last part is nontrivial, because when you look up how to push properly, there are many web pages telling you never to force push to a public repository. I am glad to receive the advice to force push here, because otherwise I would never have done it. And I'm still uncertain whether I've screwed something else up in doing so.

Nevertheless, I am hoping this finally passes travis. All tests passed on my machine, and the exact same changes passed travis before. I do not have any reason to believe the more recent failure had anything to do with me.

@nalimilan
Copy link
Member

Don't worry about force-push for PRs; there's no risk when you don't have commit access to the full repo. :-) FWIW, I push git push origin +mypr instead of git push --force origin mypr: that way, you ensure you won't accidentally overwrite another branch than the one you're pushing.

stevengj added a commit that referenced this pull request Oct 19, 2015
added print_range for Range objects, to show the elements of the Rang…
@stevengj stevengj merged commit 7a5fd44 into JuliaLang:master Oct 19, 2015
@stevengj
Copy link
Member

Thanks for persisting on this, @artkuo!

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.

3 participants