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

Pretty print diagnostics #122

Merged
merged 10 commits into from
Oct 14, 2020
Merged

Pretty print diagnostics #122

merged 10 commits into from
Oct 14, 2020

Conversation

carson-katri
Copy link
Member

The output from the Swift compiler can be pretty verbose. This adds better formatting and syntax highlighting to the compiler's output (modeled after the error messages of React).

Screen Shot 2020-10-06 at 3 56 56 PM

I also modified the screen-clearing to reset the scrollback so you can easily find the first error without scrolling past it.

@carson-katri carson-katri added the enhancement New feature or request label Oct 6, 2020
@ie-ahm-robox

This comment has been minimized.

case .call, .dotAccess, .property, .type: return "[94m"
case .number, .preprocessing: return "[33m"
case .string: return "[91;1m"
default: return "[0m"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do these values match some specific colors? I also wonder how this looks in black text on white background terminals.

Would be great to provide a link to some docs that clarify how these values are inferred.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tried with light mode, and found the ERROR text shows with black text on red, which I'll fix, but everything else looked good.

Theres's an article on Wikipedia about ANSI escape codes (section on Colors) I'll include a reference to it in the file.

Copy link
Collaborator

@MaxDesiatov MaxDesiatov 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 magical! I didn't even think we could do this 🤯

Other than nitpicks highlighted by the bot, I don't have much to add. Would be great to know how it behaves if you redirect all output to a file or a pipe? This is a common case actually, as this is equivalent to how output works if you run any carton command within GitHub Actions.

Maybe we could use something like LoggingGitHubActions in the future? This point is obviously out of scope for this PR, I just wonder how we would scale to support that case in the future? We probably should use SwiftLog for all output, but I don't have a clear picture in my head of how to integrate that with colored terminal output and other things we do, such as progress bars and clearing output.

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Oct 6, 2020

One concern I have is that this might break a certain behavior in VSCode that I constantly rely on. You can cmd-click in current diagnostics output in the integrated terminal to jump to a relevant location, as VSCode parses all strings in file:line format for that to work. That probably won't matter as soon as swiftwasm/swift#1935 is resolved and SourceKit-LSP is working properly for SwiftWasm apps in VSCode.

@ie-ahm-robox

This comment has been minimized.

@carson-katri
Copy link
Member Author

I could put the whole file path in the heading text with the first error's line #. Would that work?

@MaxDesiatov
Copy link
Collaborator

It should work. Let's try that, I hope it doesn't ruin how nice the output looked on your screenshot 😄

@MaxDesiatov
Copy link
Collaborator

And if it does look weird or incoherent if you add full path and a line of first error, I'd be totally fine with hiding this behind a CLI flag. I'd use that flag, while the rest of the users could still enjoy nicely formatted output 😁

@carson-katri
Copy link
Member Author

carson-katri commented Oct 6, 2020

Ended up putting the full path next to the filename in gray. Here's the output from VSCode. It's cmd-clickable.

Screen Shot 2020-10-06 at 5 53 34 PM

(it's not fully functional yet tho)


fileprivate static let highlighter = SyntaxHighlighter(format: TerminalOutputFormat())

func parse(_ output: String, _ terminal: InteractiveWriter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • 🚫 Function body should span 50 lines or less excluding comments and whitespace: currently spans 72 lines (function_body_length)

@ie-ahm-robox

This comment has been minimized.

@swiftwasm swiftwasm deleted a comment from ie-ahm-robox Oct 6, 2020
@swiftwasm swiftwasm deleted a comment from ie-ahm-robox Oct 6, 2020
@swiftwasm swiftwasm deleted a comment from ie-ahm-robox Oct 6, 2020
@ie-ahm-robox

This comment has been minimized.

@ie-ahm-robox

This comment has been minimized.

Package.swift Show resolved Hide resolved
Package.swift Outdated Show resolved Hide resolved
yonihemi
yonihemi previously approved these changes Oct 14, 2020
@MaxDesiatov
Copy link
Collaborator

@carson-katri ready for merging 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants