Skip to content

Modify PrettyPrinter to output diagnostics using shortened relative paths #934

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

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

TTOzzi
Copy link
Member

@TTOzzi TTOzzi commented Feb 15, 2025

Resolve #818

Modified PrettyPrinter's diagnostic logic to output diagnostics with shortened relative paths instead of full paths, as I believe this is more appropriate.

@TTOzzi TTOzzi force-pushed the fix-pretty-printer-diagnose-location branch from bdc97f6 to 8890b69 Compare February 15, 2025 14:59
@@ -819,7 +819,7 @@ public class PrettyPrinter {
context.findingEmitter.emit(
message,
category: category,
location: Finding.Location(file: context.fileURL.path, line: outputBuffer.lineNumber, column: column)
location: Finding.Location(file: context.fileURL.relativePath, line: outputBuffer.lineNumber, column: column)
Copy link
Member

Choose a reason for hiding this comment

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

I’m a little skeptical of this. When forming a URL, we provide a baseURL and a path relative to it. Generally, I wouldn’t expect it to matter what’s part of the base URL and what’s part of the relative path. For example, I would have expected url = URL(string: url.absoluteString) to not have any effect on the output but with this change it does.

What I’d prefer instead is some way of stripping off a known base path from context.fileURL here (though I don’t have concrete suggestions of how to do so at the moment).

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, this change would bring the pretty printer to be consistent with how we're using the relativePath property elsewhere.

When we create the SourceLocationConverter that the other non-pretty-printer rules use in Context.swift, we use the relativePath property.

Then, in FileIterator.swift, we have a bunch of logic that explicitly constructs the URLs this way. (In fact, I thought we were using FileManager.DirectoryEnumerationOptions.producesRelativePathURLs here, but that doesn't seem to be the case. That might have been historical, or there might have been a platform-specific issue that prevented it.)

Copy link
Member Author

Choose a reason for hiding this comment

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

As @allevato mentioned, other parts of the codebase already use relativePath except for PrettyPrinter, so I thought it would be fine to change it to use the same property for consistency.

Would it be better to implement a separate logic to derive relative path, similar to what's done in FileIterator, and apply it across all rule diagnostics, including PrettyPrinter?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, OK. I didn’t realize that we already used relativePath elsewhere. While I have the same objection still in principle, I don’t object to adding another use for it if there’s already precedence.

@ahoppen ahoppen enabled auto-merge February 19, 2025 20:51
@ahoppen ahoppen merged commit 6e1b407 into swiftlang:main Feb 19, 2025
19 checks passed
@TTOzzi TTOzzi deleted the fix-pretty-printer-diagnose-location branch March 7, 2025 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint is inconsistent on how source paths are reported
3 participants