-
Notifications
You must be signed in to change notification settings - Fork 123
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
Improve default diagnostic formatter #535
Improve default diagnostic formatter #535
Conversation
2a938f5
to
128c7f0
Compare
I love it! I've always found the primarily tool parsable diagnostics of apple's proprietary tools (docc having been open sourced but originally being an internal tool), such as FeedbackMany people use terminals with limited widths, so putting all of the suggestions for an issue after the first column of the issue (the part highlighted green) would probably mean that they get wrapped pretty often and end up being quite confusing to read. I've made a tool for reformatting the output of the Swift compiler (which is kind of similar) and here's an example of the output that it produces. It does have a lot of repetition of the issue line (so that the fixits can be displayed) but the docc suggestions don't have fixits of that style so you could possibly just have the suggestions listed like they are now in the screenshot you attached, but not indented all the way to line up with the issue. Hopefully that makes sense, I'm open to changing my opinion! (I'm not completely happy with the output my tool produces either, especially the big long arrows for the fixits, but I haven't had the time to work on it for a while) I have a feeling we both took inspiration from the Rust compiler diagnostics 😅 |
This looks super nice @arthurcro. I also like that this gives us more space to provide more detailed information about diagnostics if we'd like to in the future. What does the experience look like if the terminal doesn't support colors/formatting? |
Hi @stackotter! Thank you for the feedback, it's greatly appreciated! I agree that lines wrapping due to limited widths could be an issue when displaying multiple suggestions under each other. I was inspired by swiftlang/swift-syntax#874 which does a great job at displaying multiple issues on a same line of content. Although it does not address the line wrapping issue. I believe that duplicating the same line of content for each suggestion might be redundant and could potentially make it more challenging to discern the surrounding context. What are your thoughts on this? |
Hi @franklinsch! Thank you for you feedback, I'm super excited for this improvement! 😊 Ansi annotations, responsible for colors and formatting, will be applied only when the terminal supports them. I find it more challenging to find the suggestions within the lines of contents although the absence of line number and the "suggestion" prefix help a bit. |
One small change to improve the situation where the terminal doesn't support colors and styled text would be to use a different character than |
Thanks for the feedback! You're right, using a different character for the highlighted line significantly improves readability. I've incorporated this change into the draft PR. 🙂 I also just updated the implementation to make sure we highlight the output only when supported. I was inspired by https://github.com/apple/swift-syntax/blob/560df352d048295cc451e806eb30f7d79021d617/Sources/swift-parser-cli/TerminalUtils.swift#L27. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good to me. All the feedback I have is very minor suggestions and aren't blocking.
The only change that I'd request is to not make the changes to the DiagnosticsConsoleWriter initialized public because I think we'd like to change that again in the future and it'd be nice to not need to make that a public breaking change.
baseURL: URL? = nil, | ||
highlight: Bool? = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This to me highlights an issue with the existing abstraction for how formatters and formatting options are defined (this issue existed before your PR, it's just more noticeable now).
I don't think it's necessary (or even preferable) to fix the abstraction in this PR but I would like to make it easier to fix the abstraction later. To make that easier I think it would be good to keep the previous public initializer unchanged and add a new internal initializer that accept all 4 arguments. That way, we can change how the base URL and highlighting are configured in the future without breaking public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't feedback on this PR but rather some additional thoughts about how this abstraction could be improved in the future.
Here are some of the issues that this is making me notice:
- the base URL and highlighting are only applicable to one
initializerformatter - the only existing
DiagnosticFormattingOptions
value is used to configure which formatter is used, not how that formatter is configured. DiagnosticFormattingOptions
can't really hold configuration with values (like the URL)
I don't know what would be a better abstraction for this. I think that each formatter should have their own configuration so that it's not necessary or possible to specify configuration which will be ignored because it doesn't apply). Having separate configuration for each formatter would also make it easier to disallow conflicting configuration.
One way to accomplish this could be to take the formatter as an argument but I'm hesitant to make the formatter a public type since I don't feel that it's API is refined enough to solidify yet. One way to workaround that could be to use different (public) option types for each formatter. It's also possible that work done by the DiagnosticConsoleWriter
is simple enough that the formatter abstraction isn't useful and that each formatter should be a DiagnosticFormattingConsumer
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great point, thank you for highlighting this! I update the pull request with the changes mentioned to make the abstraction improvements easier later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessary (or even preferable) to fix the abstraction in this PR but I would like to make it easier to fix the abstraction later. To make that easier I think it would be good to keep the previous public initializer unchanged and add a new internal initializer that accept all 4 arguments. That way, we can change how the base URL and highlighting are configured in the future without breaking public API.
@d-ronnqvist I noticed that a public initializer is required if we want to set the baseUrl
in both ConvertAction
and IndexAction
since they are defined in SwiftDocCUtilities
. The baseUrl
is used to show the relative path of the source files.
This is currently failing the build as it needs to be updated. I just wanted to confirm that this is what we want or we want to make it available to those actions? After I get confirmaiton I'll push a commit to fix this and the build should pass! 🙂
Sources/SwiftDocC/Infrastructure/Diagnostics/DiagnosticConsoleWriter.swift
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Diagnostics/DiagnosticConsoleWriter.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Diagnostics/DiagnosticConsoleWriter.swift
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Diagnostics/DiagnosticConsoleWriter.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Diagnostics/ANSIAnnotation.swift
Outdated
Show resolved
Hide resolved
Sources/SwiftDocC/Infrastructure/Diagnostics/DiagnosticConsoleWriter.swift
Show resolved
Hide resolved
Tests/SwiftDocCTests/Diagnostics/DiagnosticConsoleWriterTests.swift
Outdated
Show resolved
Hide resolved
return result | ||
} | ||
|
||
private func readSourceLines(_ url: URL) -> [String] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this would be able to read the relevant lines from a symbol's in-source documentation comment if the original source file wasn't there. When DocC is used as part of a build workflow the original source files would be present at their original locations but it's also possible to build documentation with a symbol graph file without the original source being there (and the symbol graph file contains a copy of what each symbol's documentation comment was).
I don't think this needs to be fixed in this PR but it would be good with a todo comment that mentions adding support for getting the source lines from the symbol graph files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, I did not consider this scenario. I added a todo comment to add support for this at a later time!
Tests/SwiftDocCTests/Diagnostics/DiagnosticConsoleWriterTests.swift
Outdated
Show resolved
Hide resolved
To make it easier to fix the formatter abstraction later, we keep the previous public initializer unchanged and add a new internal initializer that accept all 4 arguments.
ef82af5
to
6e1d037
Compare
Since the sourceLines could potentially be big if there were diagnostics in many large files, we remove the cached lines after the console writer finished writing the diagnostics.
4ac08ae
to
2d685f1
Compare
Sorry for the late reply, now that I think about it, this simple change would basically address my main readability concerns 👍 looks good to me |
Sources/SwiftDocC/Infrastructure/Diagnostics/DiagnosticConsoleWriter.swift
Outdated
Show resolved
Hide resolved
Introduces an API to inform the formatter that all diagnostics have been formatted.
I think this probably looks good to me to merge to main. It'd be good for someone else on the team to also look at this. |
Apologies for the delay. People have been busy with other responsibilities around WWDC and are catching up on things. I'll try and see if someone else can also review this—I'm mostly looking to see if other people have thoughts on the output formatting—otherwise I'll do a second pass of the code and people can iterate on the output formatting after it's merged. |
No worries @d-ronnqvist! I figured you all were probably busy with other things! I appreciate the heads up 😊 |
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay. This looks good to me. Thank you.
I'm marking this as approved even though there are two remaining actions:
- A Linux specific build failure (see this comment)
- Merge the latest changes from
main
(the "Update branch" button)
This won't need another review after those changes.
The previous import solution was working only for Darwin platforms, it needed to be update to support more platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only some small suggestion
Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/TestTutorial.tutorial
Outdated
Show resolved
Hide resolved
Tests/SwiftDocCTests/Test Bundles/TestBundle.docc/TestTutorial.tutorial
Outdated
Show resolved
Hide resolved
@swift-ci please test |
A trailing whitespace is added in `TestTutorial.tutorial:42`, this is expected and should not have been removed.
Can't build it successfully both on my local machine and the CI env. See https://ci.swift.org/job/pr-swift-docc-macos/879/console cc @arthurcro |
@Kyle-Ye Yes, thanks for the heads up. It's because of #535 (comment). |
Sorry, I missed when I made that comment that the initializer was called from another module. Given that, I think it'd make the most sense to make the initializer public in this PR. In my opinion, adding and later removing public initializer like this one isn't "bad" enough to be worth adding other workarounds for. |
This reverts commit 6e1d037.
@swift-ci please test |
@swift-ci please test |
@arthurcro I rebased and merged this for you. Thanks for all your work on this! |
@d-ronnqvist Thank you for all the support! 😊 I look forward to contributing further to DocC! |
Bug/issue #, if applicable: #496
Summary
Hey there! 👋
After some great discussions on #496, I'm opening this pull request to improve the default diagnostic output formatting.
At present, the default diagnostic output shares the same format as the one designed for tool parsing. Consequently, it can be challenging for people to read and comprehend diagnostic output on the command line.
We believe there is significant potential to improve the readability of diagnostic output that is not intended for tool parsing. This pull requests makes the following changes:
Notes
Testing
To test this, you can run the following:
swift run docc convert Tests/SwiftDocCTests/Test\ Bundles/TestBundle.docc/
To get a wider variety of diagnostics, you can modify files in
TestBundle.docc
.Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/test
script and it succeeded