-
Notifications
You must be signed in to change notification settings - Fork 269
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
chore(ffi): add a message focused timeline variant that allows focusing on a particular event #4561
base: main
Are you sure you want to change the base?
chore(ffi): add a message focused timeline variant that allows focusing on a particular event #4561
Conversation
ca1baf0
to
10589e5
Compare
…ng on a particular event.
10589e5
to
c935351
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4561 +/- ##
==========================================
+ Coverage 85.40% 85.43% +0.02%
==========================================
Files 285 285
Lines 32218 32218
==========================================
+ Hits 27516 27524 +8
+ Misses 4702 4694 -8 ☔ View full report in Codecov by Sentry. |
fn message_filtered_timeline_builder( | ||
&self, | ||
internal_id_prefix: Option<String>, | ||
allowed_message_types: Vec<RoomMessageEventMessageType>, | ||
date_divider_mode: DateDividerMode, | ||
) -> TimelineBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit weird to have a function returning a builder, but with optional arguments. Those optional arguments could become setters on the builder itself, instead, and could be exposed in the public API. This way, we could allow for more control over how the timelines are created, with a likely simpler API. What you made here works, but will need to change the next time anyone requires a new configuration. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a tad confused, don't think I understand what you mean.
Why is it weird? It's just a private helper method to reuse configuration code between the 2 different message filtered timelines🤔
The only optional argument here is with_internal_id_prefix
which is already part of both the builder and the existing timeline creation methods. All message_filtered_timeline_builder
is make is so it doesn't need to be unwrapped in 2 different place.
Either way, this PR is not about refactoring how timelines are created. If you wish we can tackle that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have N different parameters to configure how a timeline is created. If each parameter can be configured in 2 ways, that's 2**N possible combinations overall.
Instead of having two of these possible combinations made available, with two functions, I'm suggesting to offer a way to configure each parameter independently of the others, and thus avoid proliferation of constructor functions.
Either way, this PR is not about refactoring how timelines are created. If you wish we can tackle that separately.
I dare to disagree here; the fact we need to add more adhoc constructors is truly revealing that this code needs to be refactored, so as to offer the better APIs we're aiming at here. And if we extended the thinking of "this is not about refactorings", we'd never refactor, as feature developments would always be more urgent/important, and we'd keep on piling up quick workarounds and hacks on top of existing ones, without taking the time to reflect and make the code both simpler and provide better APIs by orders of magnitude.
What it says on the tin. Fixes #4554