analyze: Fix new lints on latest Flutter main#1739
Conversation
chrisbobbe
added
the
integration review
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks for taking care of this! Comments below.
lib/model/channel.dart
Outdated
| if (!role.isAtLeast(UserRole.member)) return false; | ||
| return role == UserRole.member | ||
| ? hasPassedWaitingPeriod(user, byDate: byDate) | ||
| : true; | ||
| return role != UserRole.member | ||
| || hasPassedWaitingPeriod(user, byDate: byDate); |
There was a problem hiding this comment.
The new code here seems more confusing than the old. I feel like to understand it, I have to unpack it back into basically what we have now.
So this looks like an overzealous lint rule to me. But probably easier to accommodate it than to ignore it.
How about:
if (!role.isAtLeast(UserRole.member)) return false;
if (role == UserRole.member) {
return hasPassedWaitingPeriod(user, byDate: byDate);
}
return true;| // Crunchy-shell validation | ||
| if ( | ||
| result.flag == MessageFlag.read | ||
| && true // (we assume `event_types` has `message` and `update_message_flags`) | ||
| ) { | ||
| // (we assume `event_types` has `message` and `update_message_flags`) | ||
| if (result.flag == MessageFlag.read) { | ||
| result.messageDetails as Map<int, UpdateMessageFlagsMessageDetail>; |
There was a problem hiding this comment.
Here I don't really understand what the comment was saying about this true 🙂, so the lint seems helpful.
Looking at the API docs (https://zulip.com/api/get-events#update_message_flags-remove), I don't understand the comment now either. It looks like messageDetails is supposed to be there if the flag is "read". What's the other condition about?
There was a problem hiding this comment.
Ah: looks like the doc used to say message_details was "Present if message and update_message_flags are both present in event_types and the flag is read and the op is remove." But the event_types part was deleted in zulip/zulip@48a1cf0.
There was a problem hiding this comment.
Aha — thanks for tracking that down.
|
Thanks for fixing this. I think this is the reason #1738 is also failing. I didn't expect that "info-level" warning would also break CI! |
|
Yeah, when the PR itself introduces an info-level warning, we want to know that so we fix it before merging the PR — that's how we keep the tree in a state where it's totally clean of warning noise. But then this becomes one of several ways that a change upstream can cause our CI to start failing for all PRs. The root cause of that really is the fact that we run CI on the latest upstream. Started a chat thread on that: #mobile-team > CI on latest upstream |
… doc The doc for message_details on this event says the field is present if the flag is "read", with no mention of event_types. Looking through zulip/zulip's Git history, it turns out it had been mentioned in the past, but the mention was removed, in zulip/zulip@48a1cf04d: - Present if `message` and `update_message_flags` are both present in - `event_types` and the `flag` is `read` and the `op` is `remove`. + Only present if the specified `flag` is `"read"`. So the comment (and no-op `&& true`) isn't necessary; remove it. We noticed this because the analyzer started flagging an info-level `no_literal_bool_comparisons` here.
`flutter analyze` has started giving the following, which breaks CI:
info • Unnecessary comparison to a boolean literal • lib/api/model/events.dart:1177:10 •
no_literal_bool_comparisons
info • Unnecessary comparison to a boolean literal • lib/model/channel.dart:158:13 •
no_literal_bool_comparisons
We fixed the first one in the previous commit; this fixes the
second.
See discussion:
https://chat.zulip.org/#narrow/channel/243-mobile-team/topic/CI.20on.20latest.20upstream/near/2228858
chrisbobbe
force-pushed
the
pr-fix-new-lints
branch
from
6896eeb to
0ce94c4
Compare
|
Thanks for the review and that explanation! Revision pushed. |
|
Thanks! Looks good; merging. |
3 participants
flutter analyzehas started giving the following, which breaks CI:info • Unnecessary comparison to a boolean literal • lib/api/model/events.dart:1177:10 •
no_literal_bool_comparisons
info • Unnecessary comparison to a boolean literal • lib/model/channel.dart:158:13 •
no_literal_bool_comparisons
Fixed.