Tags : introduce set_is_favorite and set_is_low_priority #3075
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3075 +/- ##
=======================================
Coverage 83.71% 83.72%
=======================================
Files 223 223
Lines 23395 23425 +30
=======================================
+ Hits 19585 19612 +27
- Misses 3810 3813 +3 ☔ View full report in Codecov by Sentry. |
crates/matrix-sdk/src/room/mod.rs
Outdated
| @@ -937,6 +937,20 @@ impl Room { | |||
| self.client.send(request, None).await | |||
| } | |||
|
|
|||
| /// Update the tags from the room by calling set_tag or remove_tag method if | |||
| /// needed. | |||
| pub async fn update_notable_tags(&self, notable_tags: RoomNotableTags) -> Result<()> { | |||
There was a problem hiding this comment.
I'm not sure this is best to do like this for the future, or offer method for each tags like
set_is_favorite(bool)
set_is_low_priority(bool)
There was a problem hiding this comment.
Me neither. @bnjbvr do you have an opinion?
There was a problem hiding this comment.
I don't find it likely to change multiple ones at the same time, this shouldn't happen quite often so performance isn't critical, and having a single method for each notable tag makes for a better API IMO, so I'd in favor of having set_is_... methods 👍
There was a problem hiding this comment.
Do you think it's worth checking the local value tags before sending the request?
I guess we can have some race condition (especially with this issue) which could prevent the request to happen when it should.
There was a problem hiding this comment.
This is great, thank you. Please could you split the test up to make it easier to understand?
I would recommend:
- Several little helper methods e.g.
mock_favouriteto do the mocking stuff,do_sync(or a better name!) to do the sync stuff - Break it into several tests with names like
updating_notable_tags_causes_new_favourite_to_be_produced(or something better)
I really like my main test methods to be easily readable as "set up, do something, assert something" ("Given", "When", "Then" according to some people). At the moment, it takes a lot of reading to see what the test is really testing.
Thank you!
Honestly I wrote the tests in the same way other tests have been written, but I agree they are not easy to read.. |
…priority methods. Also add more tests.
|
@andybalaam ok so, I changed the method Let me know what you think! |
There was a problem hiding this comment.
The tests look much better - I was much more able to understand what you were doing - thank you!
The code looks good to me, but I will want a nod from @bnjbvr before I merge, to check this is the right approach overall.
ganfra
changed the title
| @@ -655,11 +655,16 @@ impl Room { | |||
|
|
|||
| /// Returns the current RoomNotableTags and subscribe to changes. | |||
There was a problem hiding this comment.
Please, use:
- RoomNotableTags
+ [`RoomNotableTags`]to first put RoomNotableTags in a <code>, and then to create a link to it.
There was a problem hiding this comment.
@Hywan I let you handle those changes on your own. There is tons of places where [ ] notation is not used (almost everywhere actually).
There was a problem hiding this comment.
This is your code. Can you fix it please? I'll handle the others, but please handle it in your code.
| (self.current_notable_tags().await, self.notable_tags.subscribe()) | ||
| } | ||
|
|
||
| /// Return the current RoomNotableTags extracted from store. |
There was a problem hiding this comment.
| /// Return the current RoomNotableTags extracted from store. | |
| /// Return the current [`RoomNotableTags`] extracted from store. |
crates/matrix-sdk/src/room/mod.rs
Outdated
| @@ -937,6 +937,52 @@ impl Room { | |||
| self.client.send(request, None).await | |||
| } | |||
|
|
|||
| /// Update the is_favorite flag from the room by calling set_tag or | |||
| /// remove_tag method on `m.favourite` tag. | |||
| /// If is_favorite is true, and `m.low_priority` tag is set on the room, the | |||
There was a problem hiding this comment.
| /// If is_favorite is true, and `m.low_priority` tag is set on the room, the | |
| /// If `is_favorite` is `true`, and `m.low_priority` tag is set on the room, the |
Handles "Expose a way to update tags in the bindings" from #3005