Skip to content

Commit

Permalink
Fix #10197
Browse files Browse the repository at this point in the history
  • Loading branch information
Viral B. Shah committed Feb 15, 2015
1 parent 7449714 commit 07f3ee7
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 2 deletions.
2 changes: 2 additions & 0 deletions base/exports.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export
LinAlg,
BLAS,
LAPACK,
SparseMatrix,
Docs,
Markdown,

Expand Down Expand Up @@ -904,6 +905,7 @@ export
searchindex,
show,
showall,
showarray,

This comment has been minimized.

Sign in to view

Copy link
@simonster

simonster Feb 15, 2015

Member

If we export showarray we should probably document it.

This comment has been minimized.

Sign in to view

Copy link
@ivarne

ivarne Mar 7, 2015

Member

I think we shouldn't export it at all, because it isn't usually called directly in user code, and when packages extend it, they need to extend Base.showarray anyway (if they don't do importall Base). #6117 is still open with the claim that "array display is a mess", and it won't be less of a mess if we document a single function.

This comment has been minimized.

Sign in to view

Copy link
@simonster

simonster Mar 7, 2015

Member

Yeah, I don't really think it should be exported.

This comment has been minimized.

Sign in to view

Copy link
@ivarne

ivarne Mar 7, 2015

Member

Let's hope nobody depended on it being exported dc313ab.
showcompact,
showerror,
split,
Expand Down
4 changes: 2 additions & 2 deletions base/sparse/sparsematrix.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ rowvals(S::SparseMatrixCSC) = S.rowval
nzrange(S::SparseMatrixCSC, col::Integer) = S.colptr[col]:(S.colptr[col+1]-1)

function showarray(io::IO, S::SparseMatrixCSC;
header::Bool=true, limit::Bool=Base._limit_output,
rows = Base.tty_size()[1], repr=false)
header::Bool=true, limit::Bool=Base._limit_output,
rows = Base.tty_size()[1], repr=false)
# TODO: repr?

if header
Expand Down

3 comments on commit 07f3ee7

@JeffBezanson
Copy link
Member

Choose a reason for hiding this comment

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

This is not the right fix! It shouldn't be necessary to add new Base exports in order to make code inside Base work! Ideally, sparse.jl should not contain importall Base.

@ViralBShah
Copy link
Member

Choose a reason for hiding this comment

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

SparseMatrix really should have been exported like all the other things. Not sure why that was missing. showarray also seemed like a useful thing to export, for those implementing new array types outside Base.

I do agree that otherwise this is not a good fix, but I just wanted to fix the regression first. I also do want to get rid of the importall Base in sparse.jl, which was a mental note I made when looking into this.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

mental notes would be good to record as comments...

Please sign in to comment.