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.

Sign up for GitHub

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

Jump to bottom

more consistently show lists with spaces #20288

Merged
merged 2 commits into from
Feb 1, 2017
Merged

more consistently show lists with spaces #20288

merged 2 commits into from
Feb 1, 2017

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Jan 28, 2017

#yolo

@vtjnash
be more consistent about adding spaces around low-precedence operators
6b2a684 
with the addition of the space-separate ` where ` syntax, the printing of types in lists was otherwise unbalanced

also remove multiline handling from show_delim_array. this was a relic from before multiline handling was consistent in show.
now it just (incorrectly) adds excessive newlines after multi-dimensional arrays.
@vtjnash
fix jl_static_show syntax for lb<:T<:ub
460a9d1 
if we print `lb`, then we must print `ub`, even if it does not contain essential information,
otherwise type signatures might not be able to round-trip correctly through parsing
@tkelman tkelman added the domain:display and printing Aesthetics and correctness of printed representations of objects. label Jan 28, 2017
@vtjnash vtjnash merged commit 083806c into master Feb 1, 2017
@vtjnash vtjnash deleted the jn/lists-with-spaces branch February 1, 2017 19:17
@JeffBezanson
Copy link
Sponsor Member

No strong opinion on this but I notice it still doesn't use spaces in type parameters:

julia> typeof(1=>2)
Pair{Int64,Int64}

julia> f{T,N}(x::Array{T,N})=0;

julia> methods(f)
# 1 method for generic function "f":
f{T, N}(x::Array{T,N}) in Main at REPL[10]:1

Except of course the method one will have to change anyway.

@KristofferC
Copy link
Sponsor Member

RIP doctests

This was referenced Feb 2, 2017
Closed
Merged
@KristofferC
Copy link
Sponsor Member

KristofferC commented Feb 2, 2017 •
edited
Loading

But seriously, it's pretty much impossible to work with doctests now. That this wasn't taken into considering before merging is a bit disappointing. Guess I will work on a branch where this is reverted.

@StefanKarpinski
Copy link
Sponsor Member

I suspect it didn't occur to @vtjnash that this would trash the doctests. It seems like the way to work with doctests should be to generate the new output so that you can do a git add -p and step through them interactively accept/reject changes.

KristofferC added a commit that referenced this pull request Feb 2, 2017
@KristofferC
Revert "Merge pull request #20288 from JuliaLang/jn/lists-with-spaces"
78281a0 
This reverts commit 083806c, reversing
changes made to b77bcc8.
@Keno
Copy link
Member

Keno commented Feb 2, 2017

Why aren't we running the doctests on CI?

@KristofferC KristofferC mentioned this pull request Feb 2, 2017
Merged
@ararslan
Copy link
Member

ararslan commented Feb 2, 2017

I think it has to do with how Documenter works. IIRC it's only set up to build the docs when merging to master, but I could be wrong on that. cc @MichaelHatherly

@StefanKarpinski
Copy link
Sponsor Member

Because @KristofferC, @kshyatt and @fredrikekre have been busting their butts to get the doctests to actually pass! Which this completely screws up. It breaks the doctests again and causes all their PRs to conflict.

@tkelman
Copy link
Contributor

tkelman commented Feb 2, 2017

We've never had doctests running on CI because they never stayed passing on master long enough for someone to follow that through all the way, but we were getting really close this time. Ref #19528

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Feb 2, 2017

So ... I'll make a PR adding a mode to Documenter that can auto-update the doctest output. That seems like a positive outcome? (and the work to get doctests written will be able to keep going?)

@StefanKarpinski
Copy link
Sponsor Member

See #20274 for the umbrella doctest tracking issue.

@vtjnash vtjnash mentioned this pull request Feb 2, 2017
Merged
@tkelman tkelman mentioned this pull request Apr 26, 2017
Merged
@fredrikekre fredrikekre mentioned this pull request Mar 19, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
domain:display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@vtjnash @JeffBezanson @KristofferC @StefanKarpinski @Keno @ararslan @tkelman