Add color-diagnostics/no-color-diagnostics CLI flags #8365
Conversation
johnbute
requested review from
MaxDesiatov,
bkhouri,
bnbarham,
bripeticca,
dschaefer2,
jakepetroules,
owenv,
plemarquand,
shawnhyam and
xedin
as code owners
|
Sorry for the new pr, due to a merge conflict and a mistake on my part, I had to do another pr. |
|
@swift-ci please test |
Sources/Basics/Observability.swift
Outdated
|
|
||
| // helper protocol to share default behavior | ||
| /// Helper protocol to configurate the style of diagnostics. | ||
| protocol SeverityConfig { |
There was a problem hiding this comment.
I'll reiterate the feedback I've given previously: would you clarify the purpose of this protocol? It's only used in a single place and seems redundant.
There was a problem hiding this comment.
This protocol originally was also used in testing, but I had to scrap the testing since it is difficult to test colors when you do not know the environment where the user is running these tests. As it stands, it is only used in one place. Although I agree with you that this added level of abstraction may be unnecessary, if ever someone does figure out a better way of testing color diagnostics, I believe that this protocol would help them out. Thus, for the sake of not adding test debt, I would want to keep this level of abstraction.
There was a problem hiding this comment.
I'd prefer we don't add abstractions until they are absolutely necessary. Whoever would follow up with testing can create such protocol tailored for their purposes in their own PR.
|
@swift-ci please test |
|
@swift-ci please test |
Thanks Max :) Co-authored-by: Max Desiatov <m_desiatov@apple.com>
|
@swift-ci please test |
|
@swift-ci please test windows |
|
@swift-ci please test |
|
@swift-ci please test windows |
|
@swift-ci please test linux |
| case debug | ||
|
|
||
| internal var naturalIntegralValue: Int { | ||
| var naturalIntegralValue: Int { |
There was a problem hiding this comment.
question: it does not appear this is used elsewhere. is this still needed?
There was a problem hiding this comment.
It is used in the < operator implementation below.
There was a problem hiding this comment.
sorry, but below where? searching for naturalIntegralValue did not yield any hits on changes software
There was a problem hiding this comment.
Only a dozen lines below:
There was a problem hiding this comment.
yes, but that's in unchanged code. So the variable can likely remain internal
| "\n\n" | ||
| }.terminalString() | ||
| : | ||
| plain { |
There was a problem hiding this comment.
question: Out of curiosity, why do we need two nested plain here? can this something like this
plain {
"# "
snippet.name
"\n\n"
}
| } | ||
| }.terminalString() | ||
| }.terminalString() : | ||
| plain { |
There was a problem hiding this comment.
question: do we need the two nested plain?
| }.terminalString() | ||
| "\n" | ||
| }.terminalString() : | ||
| plain { |
There was a problem hiding this comment.
question: do we need the two nested plain?
4 participants
Added the no-color-diagnostics flag to allow users to toggle on/off color diagnostics
Continuation of #8316
Motivation:
Not everyone has access to tty, or like colors in their terminal, or are color blind. This feature allows them to control if they want to see colors or not.
Modifications:
Added the color-diagnostics/no-color-diagnostics flag
Added test cases to test for colors
edited snippets to include this feature too
Result:
Developers can now control their color diagnostics when building, running, testing, installing sdks, or browsing code snippets within the swift package learn feature.