LSP default message for diagnostics #970
Conversation
… Otherwise an empty message prevents any diagnostic from appearing
WalkthroughThe recent update introduces a new method to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/config/src/rule_config.rs (1 hunks)
Additional comments: 2
crates/config/src/rule_config.rs (2)
- 70-72: The implementation of
get_default_messagemethod is straightforward and effectively returns the rule'sidas the default message. Ensure that theidfield is always populated with meaningful identifiers for this default mechanism to be useful.- 75-81: The modification to
get_messageto use a default message when the original message is empty is a good enhancement. However, consider reviewing the error handling strategy for UTF-8 conversion to ensure it aligns with the overall error handling approach of the application. Additionally, assess the performance implications of callinggenerate_replacementin this context.
crates/config/src/rule_config.rs
Outdated
| fn get_message(&self, node_match: &NodeMatch<StrDoc<L>>) -> String { | ||
| let bytes = self.message.generate_replacement(node_match); | ||
| // Note: The LSP client in vscode won't show any diagnostics at all if it receives one with an empty message |
There was a problem hiding this comment.
hmm, this is not 100% correct since the empty message will be used in sg scan CLI.
Can we move this to LSP?
There was a problem hiding this comment.
Okay I've submitted a change where it's only the diagnostic that has an empty message replaced with a default message. The message field is also used to generate the codeaction title, but vscode is happy to display a codeaction with an empty title, so I'm happy to leave the codeaction unchanged.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #970 +/- ##
==========================================
- Coverage 71.88% 71.85% -0.04%
==========================================
Files 72 72
Lines 4706 4711 +5
==========================================
+ Hits 3383 3385 +2
- Misses 1323 1326 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/lsp/src/lib.rs (2 hunks)
Additional comments: 2
crates/lsp/src/lib.rs (2)
- 190-196: The implementation of
get_non_empty_messageeffectively ensures that diagnostic messages are never empty, which aligns with the PR's objectives. However, directly using the rule'sidas a fallback message might not always provide clear context to the user. Consider adding a prefix or a more descriptive fallback message format, such as"Rule violation: " + rule.id.to_string(), to enhance clarity.- 214-214: The integration of
get_non_empty_messagewithinconvert_match_to_diagnosticis correctly implemented and aligns with the PR's objectives to ensure meaningful diagnostic messages. It's also worth reviewing the error handling and logging mechanisms within the diagnostic generation process to ensure that any issues are appropriately reported and handled.
Fix for #969
If a published diagnostic's message property would be the empty string, replace it with a default string.
The default string is the rule's id property.
Summary by CodeRabbit