Nikey646 / VndbSharp

A C# Vndb API Library. #OriginalNamingScheme
MIT License
18 stars 4 forks source link

Added a Set Votelist command, and a bugfix #5

Closed micah686 closed 7 years ago

micah686 commented 7 years ago

I fixed a small bug in your program, and added a Set Votelist command.

I would like to make sure the Set Votelist is correct before I work on creating the others.

micah686 commented 7 years ago

How should I set up the {"vote":100} field for the SetVotelist command? I'm trying to find a way to set up the fields for the Set commands, because currently, it returns an error.

Nikey646 commented 7 years ago

Hmm, it should just encoding the json correctly, unless it's getting put as a string value?

I'll take a quickly look to see if the Object data is causing issues with the Json Serialization quickly.

micah686 commented 7 years ago

It's not putting the "Byte? vote" into the json like {"vote":100}, so the request is sending something like: "set votelist 17 100" or something, instead of something like set votelist 17 {"vote": 100}"

Nikey646 commented 7 years ago

Ahh yeah, okay i see the problem. The SendRequestInternalAsync(String, UInt32, Object) method expects an anonymous object that represents the value. It would require doing something like

public async Task<Boolean> SetVotelistAsync(UInt32 id, Byte? vote)
=> await this.SendRequestInternalAsync(Constants.SetVotelistCommand, id, vote.HasValue ? new { vote } : null).ConfigureAwait(false);

This isn't very pretty, but i cannot think of a better solution to keep it minimal like the other commands (Aka: Not having to have basically the exact same code 3 times) that doesn't violate the type safety benefits gained from using this library.

Edit: For now, i've pushed the above code into the branch, if you can figure out a better solution go ahead and make a PR.

micah686 commented 7 years ago

Ok, that seems to work now. Are you going to put that in a commit yourself, or should I work on the other Set commands, then do a pull request?

Nikey646 commented 7 years ago

Appears you missed my edit :P

I did a commit myself to push it into the repo quicker for you, but if you can figure out a better solution, go ahead and make a PR~

micah686 commented 7 years ago

The wishlist is set up, but I'm not sure/still working on how to deal with the SetVnList. Due to the fact that it can have multiple fields, with one or both being null, it will require much more than a simple "vote.HasValue ? new { vote } : null" to fix this.