Use String(err) instead #428
Conversation
WalkthroughThe changes made in this pull request focus on improving the error handling logic within the Changes
Poem
Warning Rate limit exceeded@Shougo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/denops/runtime/functions/denops/request_async_test.ts(1 hunks)
🔇 Additional comments (2)
tests/denops/runtime/functions/denops/request_async_test.ts (2)
132-135: LGTM! Clear and focused type definition.
The ErrorEvent type correctly defines the structure needed for error handling with appropriate properties.
Line range hint 148-163: LGTM! Test assertion properly handles error normalization.
The test correctly uses the new normalizeMessage function and has appropriate expectations for the error message format.
| const normalizeMessage = (events: unknown[]) => { | ||
| return events.map(event => { | ||
| const error = (event as unknown[])[1] as ErrorEvent[]; | ||
| if (error[0]?.message) { | ||
| error[0].message = error[0].message.replace(/\n.*/g, ''); | ||
| } | ||
| return event; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and immutability in normalizeMessage function.
The current implementation has several potential issues:
- Unsafe type assertions without proper checks
- Potential null reference when accessing error[0]
- Mutates the input array instead of creating a new one
Consider this safer implementation:
-const normalizeMessage = (events: unknown[]) => {
- return events.map(event => {
- const error = (event as unknown[])[1] as ErrorEvent[];
- if (error[0]?.message) {
- error[0].message = error[0].message.replace(/\n.*/g, '');
- }
- return event;
- });
+const normalizeMessage = (events: unknown[]): unknown[] => {
+ return events.map(event => {
+ if (!Array.isArray(event) || event.length < 2) {
+ return event;
+ }
+ const errors = event[1];
+ if (!Array.isArray(errors) || errors.length === 0) {
+ return event;
+ }
+ return [
+ event[0],
+ errors.map(error => ({
+ ...error,
+ message: error?.message?.replace(/\n.*/g, '') ?? error?.message
+ }))
+ ];
+ });
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const normalizeMessage = (events: unknown[]) => { | |
| return events.map(event => { | |
| const error = (event as unknown[])[1] as ErrorEvent[]; | |
| if (error[0]?.message) { | |
| error[0].message = error[0].message.replace(/\n.*/g, ''); | |
| } | |
| return event; | |
| }); | |
| }; | |
| const normalizeMessage = (events: unknown[]): unknown[] => { | |
| return events.map(event => { | |
| if (!Array.isArray(event) || event.length < 2) { | |
| return event; | |
| } | |
| const errors = event[1]; | |
| if (!Array.isArray(errors) || errors.length === 0) { | |
| return event; | |
| } | |
| return [ | |
| event[0], | |
| errors.map(error => ({ | |
| ...error, | |
| message: error?.message?.replace(/\n.*/g, '') ?? error?.message | |
| })) | |
| ]; | |
| }); | |
| }; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #428 +/- ##
==========================================
- Coverage 95.55% 95.23% -0.32%
==========================================
Files 25 25
Lines 1418 1428 +10
Branches 180 181 +1
==========================================
+ Hits 1355 1360 +5
- Misses 60 65 +5
Partials 3 3 ☔ View full report in Codecov by Sentry. |
|
Could you show an example error message that shows stacktrace in PR body? |
|
Before: After: |
|
If that stack trace is all you have, it only points to the error within Denops itself and does not include the essential information about the error location in the Denops plugin. Do you have an example where the Denops plugin itself is throwing an error? |
|
For example this. |
|
I mean the error message difference itself. |
|
For example this. |
Test.
Summary by CodeRabbit