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
incendial merged 3 commits into
masterfrom
assert-for-use-setstate-sync
Feb 5, 2023
Merged

fix: support assert(mounted) for use-setstate-synchronously#1181
incendial merged 3 commits into
masterfrom
assert-for-use-setstate-sync

Conversation

@incendial

@incendial incendial commented Feb 3, 2023

Copy link
Copy Markdown
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

incendial commented Feb 3, 2023

Copy link
Copy Markdown
Member Author

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

incendial
incendial commented Feb 3, 2023
View reviewed changes
Comment thread lib/src/analyzers/lint_analyzer/rules/rules_list/use_setstate_synchronously/visitor.dart
return node.visitChildren(this);
}

node.condition.visitChildren(this);

@incendial incendial Feb 3, 2023

Copy link
Copy Markdown
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?

@Desdaemon Desdaemon Feb 3, 2023

Copy link
Copy Markdown
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.

@incendial incendial Feb 4, 2023

Copy link
Copy Markdown
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

@incendial incendial Feb 5, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

codecov Bot commented Feb 3, 2023 •
edited
Loading

Copy link
Copy Markdown

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

@dkrutskikh dkrutskikh Awaiting requested review from dkrutskikh

1 more reviewer

@Desdaemon Desdaemon Desdaemon left review comments

Reviewers whose approvals may not affect merge requirements

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