[compiler] Standarize CompilerDiagnostic descriptions (trailing periods) #34134
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mofeiZ
force-pushed
the
pr34134
branch
2 times, most recently
from
99e2886 to
4b056d3
Compare
All error messages that are not invariants, todos, or config validation errors are now moved to a registry. This helps us check that error messages and linked documentation is uniform and up to date. This also lets us link error codes to linter reporting categories and break the single `react-compiler` rule up into many smaller rules. Changes to error reporting in React Compiler: - instead of writing raw error message, add a new ErrorCode to `CompilerErrorCodes` - create errors by constructing a `CompilerErrorDetailOptions` with the correct error code, or calling `CompilerErrorDetail.fromCode()` / `CompilerDiagnostic.fromCode()`. Changes to linter: - `react-compiler` lint rule is now broken up into many smaller lint rules, each with their own test cases. - linting no longer fails early when a non-fatal error is find. Instead, we now log and move on to lint for more errors! Todos: - Codebases previously using the eslint plugin may already have `react-compiler` eslint suppressions. Since this PR removes this rule, these codebases may get new eslint warnings on already-suppressed ranges. Some options I can think of: - Force everyone to add the new suppressions - Release a script to rewrite previous suppressions to the new suppressions. Codebases that had incomplete suppressions / waited to update may benefit here, as this still surfaces errors on previously-unseen suppressions - Hard code logic in our eslint plugin to avoid reporting if we see a `eslint-disable-next-line react-compiler`. This doesn't feel ideal - Align on eslint plugin rule names and error code mappings - I aggregated a few error codes here. For example, reassigning and modifying local variables after render now have the same error code + message. See changes fixtures for more examples - More test cases for linter rules. Will add these once lint rules are more finalized
Separated this out into its own PR since it's really a cosmetic change. CompilerErrorDetail and CompilerDiagnostic now both expect `reason`/`category` and `description` to *not* have trailing periods.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Separated this out into its own PR since it's really a cosmetic change.
CompilerErrorDetail and CompilerDiagnostic now both expect
reason/categoryanddescriptionto not have trailing periods.Stack created with Sapling. Best reviewed with ReviewStack.