gamedig / rust-gamedig

Game Server Query Library.
https://crates.io/crates/gamedig
MIT License
38 stars 11 forks source link

Implement generic response as enum #53

Closed Douile closed 1 year ago

Douile commented 1 year ago

An implementation of generic response types using a raw response enum as discussed in #48 .

In principle it seems simpler and more elegant but I would like feedback as spaghetti was beginning to form:

https://github.com/Douile/rust-gamedig/blob/7dcecde1aa13c02a0d40d2eb7bbafc91a9365a9b/src/protocols/types.rs#L89-L108

CosminPerRam commented 1 year ago

This doesn't look very maintainable. How about a method that returns CommonResponse? (Let's call this to_common()) The difference between the current generic implementation and this would be getting rid of the <Protocol>ExtraResponse structs (as if you want to access them, you will access the enum data). This way we will not have to deal with the trouble of making a method for each field. What's your opinion about this?

Douile commented 1 year ago

It would still require a big match statement but at least its only one big match statement instead of many. Could have into CommonResponse for each response type like we did before to make each match branch a single line (r.into()).

I have two main questions about this:

CosminPerRam commented 1 year ago

Do we include things like players and teams on common response or give them their own method

I would say to include them on common response.

Do we implement the to_common as a method that takes ownership of the values preventing [...]

I personally would like both to be available.

Douile commented 1 year ago

Yeah i agree on having both borrow and move.

With having players and teams in the common response do we just have the basic list of names or try to construct a common struct e.g. CommonPlayer, CommonTeam?

CosminPerRam commented 1 year ago

Well, GS2 and GS3 have in common name and score, so I say yeah, the same with teams, I will look into the other implementations that are scheduled, but until we add them, you could make those common structs.

CosminPerRam commented 1 year ago

I've skimmed over through all of node's protocol implementations, and only JC2MP (Extends GS3) and UT3 (Extends GS3) have a 'Team'-like structure. Actually it seems that only GS2 and GS3 offer such informations, there are some other protocols that include informations about a team, but it's per player (such as what team they are in and whats their score), we could extract team data from players, but I don't really find this to be a viable solution, so nevermind making the Team a structure (edit: I think we shouldn't include team data in the common response).

Douile commented 1 year ago

I tried implementing as_common() and into_common() but they don't seem that maintainable.

CosminPerRam commented 1 year ago

Replaced by #56