Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_fs): improve the messaging of the "unhandled file type" diagnostic #3332

Merged
merged 2 commits into from
Oct 5, 2022

Conversation

leops
Copy link
Contributor

@leops leops commented Oct 5, 2022

Summary

Fixes #3329

This PR changes the rendering of the UnhandledDiagnostic diagnostic type, emitted by the OsFileSystem when an unknown file type is encountered. It adds an explicit error message and description, along with an information advices providing additional details. It also adds a specialized version of the error message when the unhandled file is a symbolic link, as that's the most common case for this error to be raised. Finally, it downgrades the severity of the diagnostic from error to warning, since this is not a "fatal" error that should cause the traversal to abort.

The diagnostic should now be printed as follow when encountering a symbolic link:

..\test-link\link internalError/fs ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ! Symbolic links are not supported

  i Rome does not currently support visiting the content of symbolic links since it could lead to an infinite traversal loop

In the less likely case that the CLI encounters a completely unknown file type, the following message is displayed instead:

..\test-link\unknown internalError/fs ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ! Encountered an unknown file type

  i Rome encountered a file system entry that's neither a file, directory or symbolic link

Test Plan

This diagnostic is emitted by low-level code in response to an event from the underlying file system, so it's unfortunately hard to reliably test and reproduce across platforms.

@leops leops temporarily deployed to netlify-playground October 5, 2022 09:28 Inactive
@netlify
Copy link

netlify bot commented Oct 5, 2022

Deploy Preview for rometools ready!

Name Link
🔨 Latest commit 93caa3b
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/633d95f4d29bda00091eb346
😎 Deploy Preview https://deploy-preview-3332--rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Oct 5, 2022

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Would it be possible to add some test that catches these cases? I am not sure if we can simulate a symbolic link, but we can definitely simulate the unknown file type

@leops
Copy link
Contributor Author

leops commented Oct 5, 2022

Would it be possible to add some test that catches these cases? I am not sure if we can simulate a symbolic link, but we can definitely simulate the unknown file type

The issue is that this diagnostic is emitted by the OsFileSystem only, while the tests use the MemoryFileSystem that has no concept of symbolic links or unknown file in the first place, but I'll try come up with an implementation for that.

@leops leops temporarily deployed to netlify-playground October 5, 2022 13:56 Inactive
@leops leops requested a review from ematipico October 5, 2022 14:10
@leops leops merged commit 5042251 into main Oct 5, 2022
@leops leops deleted the fix/symlink-diagnostic branch October 5, 2022 15:10
@leops leops added A-Diagnostic Area: errors and diagnostics A-CLI Area: CLI labels Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Diagnostic Area: errors and diagnostics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 Confusing error when running rome check with a directory
2 participants