gamedig / rust-gamedig

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

[Response] Add method to fetch player names #48

Closed Douile closed 1 year ago

Douile commented 1 year ago

This adds a method to optionally get a list of player names from the specific response types.

CosminPerRam commented 1 year ago

Hmmm, I don't really like the idea of cloning every String, would lifetimes be viable to use here?

CosminPerRam commented 1 year ago

I guess we could:

  1. Move
  2. Clone
  3. Add Lifetimes

But I'm not sure whats the best option, I would think 3.

Douile commented 1 year ago

Yeah moving wouldnt really work as would lose all the data in request. Will do a lifetimes implementation.

CosminPerRam commented 1 year ago

Hey! Sorry for the wait. What's your opinion on making this as a field in the GenericResponse structure? I'm not saying it should be a field instead of a function, but it's just something I thought. Also what about getting team names? Some protocols provide team names.

Douile commented 1 year ago

I chose to use a method instead of field because fields on generic response are just moving data around into different places to make it more easily accessible, in order to get a list of player names slightly more work is required: iterating the players array hence I made it optional to do this work.

Looking at the response type reference only one protocol has a teams array (although others might include it in the player data I didn't check), although even with just one it might be useful to have so developers don't need to write their own match statement.

It's a tough call in my opinion as all the data is included within the specific response enum, but its hard to decide which pieces of data should be on generic struct, which should have a method to fetch, and which we should leave only in extra data. My initial thought is to:

Although I have kind of broken these rules by converting number types for generic response.

This also makes me think whether it would've been a better idea to not have a "GenericResponse" struct type but rather use an enum of all response types (like SpecificResponse but with the actual response types instead of ExtraResponse) then implement generic properties as methods on the enum.

CosminPerRam commented 1 year ago

only one protocol has a teams array

GS2 also has it, I forgot to update the table. Will do today. Updated.

My initial thought is to [...]

Fair enough, good points.

Although I have kind of broken these rules by converting number types for generic response.

I will make a issue regarding that to set some rules regarding number types for all responses.

This also makes me think whether it would've been a better idea to not have a "GenericResponse" struct type but rather [...]

Hmmm, this sounds very good, will you look into it?

CosminPerRam commented 1 year ago

Would #53 replace this PR then?

Douile commented 1 year ago

Yes, if players is included in common response this will be replaced.

Douile commented 1 year ago

Replaced by #56