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_with_color #25522

Merged
merged 1 commit into from
Jan 27, 2018
Merged

Deprecate print_with_color #25522

merged 1 commit into from
Jan 27, 2018

Conversation

ararslan
Copy link
Member

print_with_color is an odd function; its name is rife with underscores and it violates our argument order conventions by putting the color before the I/O argument.

This PR replaces print_with_color with a more general function called printstyled, which accepts the color as a keyword argument to mimic the bold keyword. That is, print_with_color(:red, "hi", bold=true) is now printstyled("hi", color=:red, bold=true).

There was also an unexported Base.println_with_color, which has been removed entirely without deprecation.

@ararslan ararslan added display and printing Aesthetics and correctness of printed representations of objects. deprecation This change introduces or involves a deprecation labels Jan 12, 2018
@ararslan ararslan requested a review from JeffBezanson January 12, 2018 00:39
Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

This is certainly an improvement. I wonder if it wouldn't make more sense to just have a generic Style(content, color=:cyan, bold=true) wrapper for content – where the content could be a bare string or another Style object or whatever and then you just call print on that. @KristofferC may have some thoughts as the author of Crayons...

@ararslan ararslan force-pushed the aa/print_with_color branch from 28ffa96 to 02bebf4 Compare January 12, 2018 01:37
@KristofferC
Copy link
Member

The Style method is similar to how Crayon does it.

If we change print_with_color we should probably change with_output_style in the same way. Even though it is unexported it is frequently used outside Base (because it is quite useful).

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jan 12, 2018

Should we just import (steal) parts of Crayons to replace this? print_with_color is exported (I had to check).

@KristofferC
Copy link
Member

I can make a PR with a suggestion (after I have moved out REPL to stdlib). The name "Crayons" is perhaps a bit too cute for an stdlib but it could easily be renamed to something more descriptive.

@ararslan
Copy link
Member Author

Something that's been suggested was to wrap the I/O argument in IOContext and supply color and bold information to the constructor. One issue with that is that IOContext already has color property that means something else: it's a Bool denoting whether the wrapped IO supports color (IIRC). It would be possible to deprecate that name to something like hascolor in 0.7, then introduce color to specify a color in 1.0, but I'm not sure that print(IOContext(STDOUT, color=:blue, bold=true), "hi") is actually a friendlier API.

@JeffBezanson
Copy link
Member

It doesn't fully make sense to me that formatting style would be part of the IO context.

@ararslan ararslan mentioned this pull request Jan 17, 2018
19 tasks
@JeffBezanson JeffBezanson added the triage This should be discussed on a triage call label Jan 23, 2018
@JeffBezanson
Copy link
Member

JeffBezanson commented Jan 25, 2018

Let's just rename this to printstyled.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Jan 25, 2018
@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jan 25, 2018
@StefanKarpinski
Copy link
Member

Triage dixit: go with this PR (rename to printstyled) and merge. Something fancier could be added in the future but this can exist as a legacy interface.

@ararslan ararslan force-pushed the aa/print_with_color branch from 02bebf4 to 1092f83 Compare January 25, 2018 21:55
This deprecates print_with_color in favor of printstyled, which accepts
the color as a keyword argument.
@ararslan ararslan force-pushed the aa/print_with_color branch from 1092f83 to 97e2054 Compare January 26, 2018 21:48
@ararslan
Copy link
Member Author

ararslan commented Jan 27, 2018

CI summary:

System Status
Travis Linux x86
Travis Linux x64
Travis macOS Timeout
AppVeyor x86 Timeout
AppVeyor x64
Circle x86
Circle x64 Unrelated
FreeBSD

@ararslan ararslan merged commit 9877594 into master Jan 27, 2018
@ararslan ararslan deleted the aa/print_with_color branch January 27, 2018 02:41
@fredrikekre fredrikekre added the needs news A NEWS entry is required for this change label Mar 19, 2018
ssfrr added a commit to ssfrr/julia that referenced this pull request Jun 24, 2018
the `needs_news` tag can be removed from JuliaLang#25522
@ssfrr
Copy link
Contributor

ssfrr commented Jun 24, 2018

Triage dixit: go with this PR (rename to printstyled) and merge. Something fancier could be added in the future but this can exist as a legacy interface.

In the future could bold and color be added as keyword args to print rather than having a separate function, or does that cause some problems I'm not thinking about?

@ViralBShah ViralBShah removed the needs news A NEWS entry is required for this change label Jun 25, 2018
fredrikekre pushed a commit that referenced this pull request Jun 25, 2018
* adds `printstyled` to NEWS.md

the `needs_news` tag can be removed from #25522

* fixes `printstyled` trailing whitespace in NEWS.md
jrevels pushed a commit that referenced this pull request Jul 2, 2018
* adds `printstyled` to NEWS.md

the `needs_news` tag can be removed from #25522

* fixes `printstyled` trailing whitespace in NEWS.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation display and printing Aesthetics and correctness of printed representations of objects.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants