Skip to content

Conversation

@connorshea
Copy link
Contributor

Minor improvements for the diagnostics used in this rule.

Copilot AI review requested due to automatic review settings December 14, 2025 02:13
@connorshea connorshea requested a review from camc314 as a code owner December 14, 2025 02:13
@github-actions github-actions bot added A-linter Area - Linter C-docs Category - Documentation. Related to user-facing or internal documentation labels Dec 14, 2025
Copy link

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

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

Diagnostics are improved overall, but the new helper name is slightly unclear and the new message has a small grammar nit (“in a global scope” vs “in the global scope”). These are minor, user-facing polish items and don’t affect correctness.

Additional notes (1)
  • Readability | crates/oxc_linter/src/rules/jest/no_confusing_set_timeout.rs:15-17
    The function name non_global_set_timeout_diagnostic reads a bit awkwardly and is slightly ambiguous (it could be interpreted as “a diagnostic about non-global setTimeout” vs “diagnostic for setTimeout not in global scope”). Since the message is specifically about scope, a name that encodes that intent tends to be easier to scan and maintain.
Summary of changes

Summary

  • Renamed the diagnostic helper from no_global_set_timeout_diagnostic to non_global_set_timeout_diagnostic.
  • Improved the warning message for non-global jest.setTimeout usage to be clearer and grammatically correct.
  • Added a trailing period to the “placed before any other jest methods” warning message.
  • Updated the snapshot file crates/oxc_linter/src/snapshots/jest_no_confusing_set_timeout.snap to match the revised diagnostics.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR improves the diagnostic messages for the jest/no-confusing-set-timeout linter rule by fixing grammatical errors and improving clarity. The changes make the error messages more readable and professional.

Key changes:

  • Fixed grammatical error: "should be call" → "should only be called"
  • Improved clarity: "call in global scope" → "called in a global scope"
  • Renamed function from no_global_set_timeout_diagnostic to non_global_set_timeout_diagnostic for better semantic accuracy
  • Added period to one diagnostic message for consistency (though this creates inconsistency within the file)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
crates/oxc_linter/src/rules/jest/no_confusing_set_timeout.rs Updated diagnostic message text to fix grammar, improved function naming, and added punctuation
crates/oxc_linter/src/snapshots/jest_no_confusing_set_timeout.snap Updated snapshot file to reflect the improved diagnostic messages

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 14, 2025

CodSpeed Performance Report

Merging #16815 will not alter performance

Comparing no-confusing-set-timeout-doc (182fba2) with main (9e59413)

Summary

✅ 4 untouched
⏩ 41 skipped1

Footnotes

  1. 41 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@connorshea connorshea added the 0-merge Merge with Graphite Merge Queue label Dec 14, 2025
Copy link
Contributor Author

connorshea commented Dec 14, 2025

Merge activity

…ut rule. (#16815)

Minor improvements for the diagnostics used in this rule.
@graphite-app graphite-app bot force-pushed the no-confusing-set-timeout-doc branch from 182fba2 to bbafba5 Compare December 14, 2025 02:22
@graphite-app graphite-app bot merged commit bbafba5 into main Dec 14, 2025
20 checks passed
@graphite-app graphite-app bot deleted the no-confusing-set-timeout-doc branch December 14, 2025 02:27
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-linter Area - Linter C-docs Category - Documentation. Related to user-facing or internal documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants