Skip to content
This repository was archived by the owner on Jul 16, 2023. It is now read-only.

fix: support assert(mounted) for use-setstate-synchronously #1181

Merged
merged 3 commits into from
Feb 5, 2023
Merged

fix: support assert(mounted) for use-setstate-synchronously #1181

merged 3 commits into from
Feb 5, 2023

Conversation

incendial
Copy link
Member

Please write a short comment explaining your change (or "none" for internal only changes)

#1178

@incendial incendial added this to the 5.6.0 milestone Feb 3, 2023
@incendial incendial requested a review from dkrutskikh February 3, 2023 14:41
@incendial incendial self-assigned this Feb 3, 2023
@incendial
Copy link
Member Author

@Desdaemon please take a look, if you have a moment

incendial
incendial commented Feb 3, 2023
View reviewed changes
lib/src/analyzers/lint_analyzer/rules/rules_list/use_setstate_synchronously/visitor.dart
@@ -65,7 +72,6 @@ class _AsyncSetStateVisitor extends RecursiveAstVisitor<void> {
return node.visitChildren(this);
}

node.condition.visitChildren(this);
Copy link
Member Author

Choose a reason for hiding this comment

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

@Desdaemon removing this looks safe or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

This might cause lints to fail for e.g. if (await ..), but I was mostly being cautious when I wrote this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I'll add some tests to check this case

Copy link
Member Author

@incendial incendial Feb 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, it was not being checked coz of .visitChildren, I've added a test and fixed the problem

@codecov
Copy link

codecov bot commented Feb 3, 2023 •
edited
Loading

Codecov Report

Merging #1181 (4171bde) into master (14ffd46) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4171bde differs from pull request most recent head 8b8508c. Consider uploading reports for the commit 8b8508c to get more accurate results

@@           Coverage Diff           @@
##           master    #1181   +/-   ##
=======================================
  Coverage   86.11%   86.11%           
=======================================
  Files         359      359           
  Lines        8071     8076    +5     
=======================================
+ Hits         6950     6955    +5     
  Misses       1121     1121
Impacted Files Coverage Δ
...rules_list/use_setstate_synchronously/visitor.dart 86.40% <100.00%> (+0.40%) ⬆️
...lyzer/rules/rules_list/format_comment/visitor.dart 100.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@incendial incendial merged commit cde4648 into master Feb 5, 2023
@incendial incendial deleted the assert-for-use-setstate-sync branch February 5, 2023 15:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Desdaemon Desdaemon Desdaemon left review comments

@dkrutskikh dkrutskikh Awaiting requested review from dkrutskikh

Assignees

@incendial incendial

Labels
area-rules
Projects
None yet
Milestone
5.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@incendial @Desdaemon