refactor!(all the things): fuse SyncTimelineEvent into TimelineEvent
#4568
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4568 +/- ##
==========================================
- Coverage 85.42% 85.41% -0.01%
==========================================
Files 285 285
Lines 32222 32211 -11
==========================================
- Hits 27525 27513 -12
- Misses 4697 4698 +1 ☔ View full report in Codecov by Sentry. |
poljar
requested changes
Jan 22, 2025
| @@ -11,6 +11,9 @@ All notable changes to this project will be documented in this file. | |||
| - Replaced `Room::compute_display_name` with the reintroduced `Room::display_name()`. The new | |||
| method computes a display name, or return a cached value from the previous successful computation. | |||
| If you need a sync variant, consider using `Room::cached_display_name()`. | |||
| - [**breaking**]: The reexported types `SyncTimelineEvent` and `TimelineEvent` have been fused into a single type | |||
| `TimelineEvent`, and its field `push_actions` has been made `Option`al (it is set to `None` when | |||
| we couldn't compute the push actions, because we lacked some information). | |||
There was a problem hiding this comment.
Please add a link to the PR introducing this change.
| @@ -11,6 +11,9 @@ All notable changes to this project will be documented in this file. | |||
| - Replaced `Room::compute_display_name` with the reintroduced `Room::display_name()`. The new | |||
| method computes a display name, or return a cached value from the previous successful computation. | |||
| If you need a sync variant, consider using `Room::cached_display_name()`. | |||
| - [**breaking**]: The reexported types `SyncTimelineEvent` and `TimelineEvent` have been fused into a single type | |||
There was a problem hiding this comment.
Suggested change
| - [**breaking**]: The reexported types `SyncTimelineEvent` and `TimelineEvent` have been fused into a single type | |
| - [**breaking**]: The reexported types `SyncTimelineEvent` and `TimelineEvent` have been fused into a single type |
There was a problem hiding this comment.
I'm not against this change, but can I ask why we should do it like this? This will break the list into two lists, now, which seems counter to the point of having a list of changes.
There was a problem hiding this comment.
I don't particularly care, but if you prefer the rendering without the extra newlines, then we shouldn't have them in the other changelog either.
| @@ -6,6 +6,10 @@ All notable changes to this project will be documented in this file. | |||
|
|
|||
| ## [Unreleased] - ReleaseDate | |||
|
|
|||
| - [**breaking**]: `SyncTimelineEvent` and `TimelineEvent` have been fused into a single type | |||
| `TimelineEvent`, and its field `push_actions` has been made `Option`al (it is set to `None` when | |||
| we couldn't compute the push actions, because we lacked some information). | |||
| @@ -25,6 +25,10 @@ All notable changes to this project will be documented in this file. | |||
|
|
|||
| ### Refactor | |||
|
|
|||
| - [**breaking**]: The reexported types `SyncTimelineEvent` and `TimelineEvent` have been fused into a single type | |||
| `TimelineEvent`, and its field `push_actions` has been made `Option`al (it is set to `None` when | |||
| we couldn't compute the push actions, because we lacked some information). | |||
Eh, not so sure, we already have another |
As the comment noted, they're essentially doing the same thing. A `TimelineEvent` may not have computed push actions, and in that regard it seemed more correct than `SyncTimelineEvent`, so another commit will make the field optional.
bnjbvr
force-pushed
the
bnjbvr/unify-timeline-events
branch
from
7fc8246 to
914f274
Compare
|
Had to rebase for the new |
poljar
approved these changes
Jan 22, 2025
bnjbvr
force-pushed
the
bnjbvr/unify-timeline-events
branch
from
914f274 to
a66fa24
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The breakiest of breaking changes:
SyncTimelineEventtook aRaw<AnySyncTimelineEvent>, andTimelineEventtook aRaw<AnyTimelineEvent>that it immediately cast intoRaw<AnySyncTimelineEvent>, so we can use both.TimelineEventhad itspush_actionsoptional, which is the right call; another commit makes that change, partially reverting some changes of the first commit, sorry!SyncTimelineEventis eventually renamed toTimelineEvent, but as the last commit suggests… maybe this could be plain calledEvent? (Would likely break some serialization formats that rely on type names)