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

deprecate print_shortest? #25745

Closed
StefanKarpinski opened this issue Jan 25, 2018 · 11 comments
Closed

deprecate print_shortest? #25745

StefanKarpinski opened this issue Jan 25, 2018 · 11 comments
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc. kind:deprecation This change introduces or involves a deprecation

Comments

@StefanKarpinski
Copy link
Sponsor Member

This is kind of a weird, old legacy function that we export. Should we deprecate it? Use an IO context flag? Just unexport it?

@StefanKarpinski StefanKarpinski added domain:io Involving the I/O subsystem: libuv, read, write, etc. status:triage This should be discussed on a triage call labels Jan 25, 2018
@vtjnash vtjnash added needs tests Unit tests are required for this change and removed status:triage This should be discussed on a triage call labels Jan 25, 2018
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 25, 2018

Triage decided the code can remain in Grisu, but it shouldn't be exported anymore

@Keno
Copy link
Member

Keno commented Jan 25, 2018

Triage: We should un-export this from base, but keep it available in Grisu if people want to use it directly. Using Grisu directly is sometimes a reasonable thing to do if you want really custom float printing, e.g. the plotting packages use it to print data points with consistent precision.

@strickek
Copy link
Contributor

print_shortest is also used in writedlm(), line 733 of DelimitedFiles in stdlib.

@StefanKarpinski
Copy link
Sponsor Member Author

As part of the stdlib it's fairly kosher for that to depend on an internal Base function since they will be updated in sync.

@JeffBezanson
Copy link
Sponsor Member

This could become a :shortest flag in IOContext. Grisu already looks at the :compact flag.

@mbauman mbauman added kind:deprecation This change introduces or involves a deprecation and removed needs tests Unit tests are required for this change labels Jan 25, 2018
@StefanKarpinski
Copy link
Sponsor Member Author

StefanKarpinski commented Jan 25, 2018

We could also just get rid of this since the only reason DelimitedFiles calls it is because I put it there and I think various people would prefer that it use more standard short-but-not-necessarily-the-tricksiest-and-very-shortest printing.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jan 25, 2018

I've been strongly reluctant to have IOContext be used to pass specific formatting choices recursively. I think if you care about that level of detail in your output, you should [have to] define the recursive structure also.

Not sure why mbauman removed needs-tests – this function does not appear to have any. If it's not simply deleted, it needs them written instead.

@StefanKarpinski
Copy link
Sponsor Member Author

StefanKarpinski commented Jan 25, 2018

This issue doesn't add any features so this is not the place to put the "needs tests" label. If you want to open a separate issue about testing print_shortest that would be fine, but that seems premature since we may decide to delete it.

@ararslan
Copy link
Member

Now that we have printstyled, we could fold shortest into a keyword argument to that function.

@StefanKarpinski
Copy link
Sponsor Member Author

Frankly, I'm just not sure this is all that necessary of a function. But sure, that's an option.

@strickek
Copy link
Contributor

I think the magic of print_shortest in writedlm() is not necessary. It lose the type differentation for floats with '*.0' because print_shortest write them as '0', so afterwards you dont know wether this element was originaly an Int or an Float.

@ararslan ararslan mentioned this issue Feb 2, 2018
19 tasks
ararslan added a commit that referenced this issue Feb 2, 2018
This deprecates `print_shortest` to `Base.Grisu.print_shortest`.

The only place where `print_shortest` is used is in DelimitedFiles, when
writing numeric values. This has been changed to `print`, which is
technically a breaking change, though it more accurately represents the
value being written.

Fixes #25745.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc. kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Development

No branches or pull requests

7 participants