-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Boilerplate error improvements #500
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
CHANGELOG.md
Outdated
@@ -1,5 +1,11 @@ | |||
# OverReact Changelog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO also add
- improved messaging for runtime errors when state is missing
- build time error/warning when state is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @joebingham-wk I plan to address these in this PR
# Conflicts: # lib/src/builder/parsing/declarations_from_members.dart
e891d39
to
8d12008
Compare
47fa376
to
e323c04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to the wording changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QA +1
@Workiva/release-management-p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from RM
Motivation
Boilerplate-related error messages need some love:
Using the wrong generated config wrong for a function component resulted in an error message that was confusing and unhelpful
Error messages relating to missing boilerplate members weren't explicit about mismatched names being the likely culprit
Many error messages on boilerplate members referenced the whole node as a span, which can be difficult to read in builder output (since the whole span is highlighted) and in the analyzer warnings (since the error can overlap other errors)
(see image for Set up CI and coverage #2)
Comments in generated code in place to help debug bad references to generated mixins were a bit vague
Changes
Update factory error messages to mention generated factory config used by function components and also include examples
When boilerplate members cannot be found, include information about checking that names match
- Also update the runtime error specific to mismatched state with this informationUpdate boilerplate error to use the name where possible instead of the whole node
(see image for Set up CI and coverage #2)
Clarify comments in generated code that bad references to generated mixins can be caused by bad imports as well as bad export
Release Notes
Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review:
QA Checklist
Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: