Skip to content

Small NFC tweaks to message-move-related code #786

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

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Jul 3, 2024

I'm about to send all my draft changes toward #150 as a draft PR, so that @PIG208 can build on that without duplicating the work I did on that a couple of months ago.

This PR pulls out from that draft work a couple of small prep commits that are self-contained and ready to go, so that we can merge them (to reduce future merge conflicts) without blocking on the larger work.

gnprice added 2 commits July 3, 2024 16:46
This lets the Dart compiler see that these are definitely constant
through the function (which it can't assume when they're getters on
an object).  That in turn lets it narrow the types based on our
null checks, so that we can leave out `!` operators later.

That's small now, but will be more useful as we add more logic to
this function -- if we had to scatter `!` all around that logic,
we'd have to wonder whether some of them were unjustified assumptions
and might throw at runtime.

Also rename one local to "newStreamId", matching the property it
mirrors.  We'll rename both the variable and the property together,
as part of a sweep coming up soon (zulip#631).
This messageContentChanged method goes logically next to the
various event-handling methods, rather than separated from them
by notifyListeners and its siblings.
@gnprice gnprice requested review from chrisbobbe and PIG208 July 3, 2024 23:50
@chrisbobbe chrisbobbe merged commit 8a4900e into zulip:main Jul 3, 2024
1 check passed
@chrisbobbe
Copy link
Collaborator

LGTM, thanks! Merged.

@gnprice gnprice deleted the pr-move-messages-minor-prep branch July 4, 2024 00:02
@gnprice
Copy link
Member Author

gnprice commented Jul 4, 2024

Thanks for the swift review!

For cross-reference, that subsequent draft PR with the main changes is #787.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants