Page MenuHomePhabricator

[lib] Create types for message editing
ClosedPublic

Authored by kuba on Mar 7 2023, 1:32 AM.
Tags
None
Referenced Files
F1436578: D6961.id24212.diff
Thu, Mar 28, 1:01 PM
F1431493: D6961.id24212.diff
Thu, Mar 28, 2:58 AM
Unknown Object (File)
Tue, Mar 26, 12:45 PM
Unknown Object (File)
Feb 22 2024, 1:19 AM
Unknown Object (File)
Feb 21 2024, 4:03 AM
Unknown Object (File)
Feb 20 2024, 11:32 PM
Unknown Object (File)
Feb 20 2024, 11:31 PM
Unknown Object (File)
Feb 20 2024, 11:31 PM

Details

Summary

Created types for new 'edit_message' message type.

Test Plan

Checked if the app still works.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Can you also updates lib/types/messages.js? EDIT ah I see these are in the next diff

In general I personally find it better to introduce types along with code that actually uses them, but many people on the team do what you did here and introduce them in a separate diff. It just makes it much harder to test... eg. your Test Plan doesn't actually test any of the types at all, which means it's quite a poor test plan.

Separately, I think it might be good to add @ginsu to your list of reviewers for this work, given how recently he touched reactions

Can you also updates lib/types/messages.js? EDIT ah I see these are in the next diff

In general I personally find it better to introduce types along with code that actually uses them, but many people on the team do what you did here and introduce them in a separate diff. It just makes it much harder to test... eg. your Test Plan doesn't actually test any of the types at all, which means it's quite a poor test plan.

Sure, I didn't know whether I should divide it or not. Next time I will do it in one diff.

This revision is now accepted and ready to land.Mar 9 2023, 9:56 AM

Removed localID from the type

Can you clarify in what scenarios the id can be null? Usually we only see that for types that also have localID

Can you clarify in what scenarios the id can be null? Usually we only see that for types that also have localID

You are right. I couldn't find such scenario. I also tested the app and it still works as expected. Updating the diff.

lib/types/messages/edit.js
16

Why isn't this readonly?