Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@goderbauer
Copy link
Member

@goderbauer goderbauer commented Mar 20, 2021

In preparation for dart-lang/sdk#35234.

@goderbauer goderbauer force-pushed the testlints branch 2 times, most recently from bf6f9a0 to 980190a Compare March 24, 2021 16:30
@goderbauer goderbauer changed the title Testlints Remove ignores that are not ignoring anything Mar 24, 2021
@goderbauer goderbauer marked this pull request as ready for review March 24, 2021 20:33
@goderbauer goderbauer requested a review from chinmaygarde March 24, 2021 20:33
@goderbauer goderbauer added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 24, 2021
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 25, 2021
@goderbauer goderbauer added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 25, 2021
@fluttergithubbot fluttergithubbot merged commit 0353b71 into flutter:master Mar 25, 2021
@goderbauer goderbauer deleted the testlints branch March 25, 2021 17:38
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 25, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 26, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 26, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 29, 2021
@flar
Copy link
Contributor

flar commented Mar 30, 2021

I'm perplexed about these changes.

Aren't the "ignore unnecessary null comparison" comments a reminder that we should remove the assert statements at some point because once NNBD becomes a mandatory requirement rather than an optional choice then they are really vestigial.

Haven't we reached that point? Isn't NNBD the mandatory the default yet? If not, shouldn't we leave the comments to draw our attention to the unnecessary asserts later? If so, then why aren't we removing the vestigial asserts instead of removing the comments?

@goderbauer
Copy link
Member Author

goderbauer commented Mar 30, 2021

Aren't the "ignore unnecessary null comparison" comments a reminder that we should remove the assert statements at some point because once NNBD becomes a mandatory requirement rather than an optional choice then they are really vestigial.

The "unnecessary null comparison" lint has been turned off for the entire project here:

# Turned off until null-safe rollout is complete.
unnecessary_null_comparison: ignore

So the line-based ignores are no longer necessary (and once dart-lang/sdk#35234 is resolved no longer allowed). Once we are ready to drop non-NNBD support, we can enable the lint again on the project level and it will tell us about all the lines that can now be removed.

Haven't we reached that point? Isn't NNBD the mandatory the default yet?

No, apps can (and many (most?) in the wild still do) run without NNBD enabled. In that mode we still need the asserts.

chandarrengoog pushed a commit to chandarrengoog/engine that referenced this pull request Mar 30, 2021
duanqz pushed a commit to duanqz/engine that referenced this pull request Apr 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants