-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix: No warning if the number of elements in the spread operator using the if statement is more than two. #74
Conversation
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.
The issues in the corresponding Issue are as follows:
Even though it contains multiple elements, the avoid_single_child warning appears when using an if condition.
Looking at the PR it looks like another revision, what is the intent?
The title and summary description of the PR did not seem to match the revisions. We have just corrected the title and summary description. The PR has been modified to display a warning when the number of elements in the if statement described in the child element is two or more, and not when the number is one. |
? [ | ||
const Text('Hello World'), | ||
] | ||
: [ | ||
const Text('Hello'), | ||
const Text('World'), | ||
], |
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.
bool _hasSingleChild(CollectionElement element) { | ||
if (element is SpreadElement && element.expression is ListLiteral) { | ||
final spreadElement = element.expression as ListLiteral; | ||
return spreadElement.elements.length != 1; | ||
} |
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.
@boywithdv
What does the name _hasSingleChild
mean when length != 1
, what does the name _hasSingleChild
mean?It seems intuitive if length == 1
...
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.
Indeed, at a quick glance, it seems that the function name does not match the actual situation.
The function name _hasSingleChild seems to return true in the case of a single element, so the function name should be changed to _hasMultipleChild to make it clear that it returns true when there are multiple child elements.
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.
Names are important. Make sure you name it with proper thought and self-review to make sure it is correct.
Co-authored-by: Ryunosuke Muramatsu <[email protected]>
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.
Thanks for adding the test!
I commented on some of them, so please check them and merge them if they are OK.
bool _hasSingleChild(CollectionElement element) { | ||
if (element is SpreadElement && element.expression is ListLiteral) { | ||
final spreadElement = element.expression as ListLiteral; | ||
return spreadElement.elements.length != 1; | ||
} |
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.
Names are important. Make sure you name it with proper thought and self-review to make sure it is correct.
Co-authored-by: Ryunosuke Muramatsu <[email protected]>
🔗 Related Issues
🙌 What's Done
✍️ What's Not Done
🖼️ Image Differences
Both if and else will similarly display a warning message if the number of elements is one.
🤼 Desired Review Method
Note
It is possible that a reviewer's will may cause a method to be implemented that is not selected.
📝 Additional Notes
Pre-launch Checklist