webxdc / store

XDC store, migrated to codeberg
https://codeberg.org/webxdc/store
5 stars 0 forks source link

Use concrete types for request variants #21

Closed link2xt closed 1 year ago

link2xt commented 1 year ago

While writing the test in #20 I noticed that update request payload has a data field which is not documented:

{"request_type": "Update", "data": 0}

Update payload is first deserialized into FrontendRequest<RequestType> and then into FrontendRequestWithData<RequestType, usize>: https://github.com/webxdc/appstore-bot/blob/b7d08f369040de6aaa483d4f2e28c32bb5d41e47/src/request_handlers/shop.rs#L127-L135

All the generic types are declared in https://github.com/webxdc/appstore-bot/blob/b7d08f369040de6aaa483d4f2e28c32bb5d41e47/src/request_handlers/mod.rs

Usage of generic data field makes it impossible to document what the data is and name it properly.

Proper Rust type for the same requests as handled now could look like this:

enum ShopRequest {
    Update {
        /// Requested update sequence number.
        serial: u64
    },
    Download {
        /// ID of the requested application.
        app_id: String
    }
}

Serde can serialize this into JSON, some examples:

{"Update": {"serial": 0}}
{"Download": {"app_id": "calendar"}}
{"Update": {"serial": 15}}

Then you can deserialize incoming payload and match on it instead of trying to deserialize it into FrontendRequestWithData<RequestType, usize> after deserializing into FrontendRequest<RequestType>.

This is more readable and self-documenting than "data" field with the type declared only in the parsing code here: https://github.com/webxdc/appstore-bot/blob/b7d08f369040de6aaa483d4f2e28c32bb5d41e47/src/request_handlers/shop.rs#L132 (I would also avoid using usize for sequences, because its actual size depends on the platform the bot is running on)

Generally speaking, I am trying to avoid using generic types (and macros) whenever possible, they are hard to understand if you don't know what the type T and R is, e.g. here: https://github.com/webxdc/appstore-bot/blob/b7d08f369040de6aaa483d4f2e28c32bb5d41e47/src/request_handlers/mod.rs#L197-L200

Septias commented 1 year ago

Yeah sounds good :+1: