Skip to content
This repository has been archived by the owner on Jun 11, 2022. It is now read-only.

Adding support to get the match history for a player #69

Closed
zettelmj opened this issue May 10, 2015 · 6 comments
Closed

Adding support to get the match history for a player #69

zettelmj opened this issue May 10, 2015 · 6 comments
Milestone

Comments

@zettelmj
Copy link

Hi,

i'm currently adding support to get the Match History for a player. Since it is working for my usecase i wanted to contribute the changes back. That is where it starts to get more interesting:
What is your approach on having multiple parameters? CMsgDOTAGetPlayerMatchHistory has a lot of parameters:

message CMsgDOTAGetPlayerMatchHistory {
optional uint32 account_id = 1;
optional uint32 start_at_match_id = 2;
optional uint32 matches_requested = 3;
optional uint32 hero_id = 4;
optional uint32 request_id = 5;
}

Granted not all of them need to be used a the same time. But all of them do make sense (with the exception of the hero_id). So would you prefere them to be exposed individually as in function a(param1, param2,...) or would you prefere function a({param1, param2, ...})?

Thanks
Jens

@jimmydorry
Copy link
Member

The array would make the most sense, as long as it is properly documented.

@rjackson rjackson added this to the 1.0.0 milestone Sep 11, 2015
@rjackson
Copy link
Member

I think this has been completed in #90. Can anybody confirm?

@howardchung
Copy link
Contributor

@pyarmak added it, I think. I've never tried to use it myself.

@Crazy-Duck
Copy link
Collaborator

The method was added, but it has fixed arguments. Might want to refactor it to take 1 filter argument instead, kinda like lobbies.

@Crazy-Duck
Copy link
Collaborator

I've put a rework on my dev branch. I externalized the option parser from the configureLobby method in a separate file -> handlers/helper.js. createPracticeLobby, configureLobby and getPlayerMatchHistory all use this method to parse their input arguments. I made sure that all obligatory arguments are required on their respective interfaces.

@Crazy-Duck
Copy link
Collaborator

This issue can be closed I think, modification has been merged some weeks ago

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants