Nikey646 / VndbSharp

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

Expanded Error codes, Added Mono warning, and created a working version of Set Votelist #4

Closed micah686 closed 7 years ago

micah686 commented 7 years ago

So I used the "HasMono" to warn programmers about the lack of security while using the SecureString class.

I also created a sample version of the "set votelist" command. I didn't use the same framework as the previous "get" commands, as the "set" commands have no filter support.

Also, I'm unsure if you wished for the set commands to return an "ok" or an error message after it tries to set something in the API. If you wish for the set commands to return an error or "ok", I'll rework it to include that. I just wanted input before I create the other "set" methods.

Nikey646 commented 7 years ago

Set commands should return a Task<Boolean> so when using them you know if they failed or not, otherwise you'll need to check the last error, which is never reset from the last error, so effectively there is no real way to check them then.

Edit: Also, while minor, I would prefer if everything went through the json serializer, even if it's a single property. You can use an anonymous object instead of a POCO here though, which will reduce clutter

var voteObj = new {
    vote = vote
}

if (vote != null)
    data += " " + JsonConvert.SerializeObject(voteObj);

Something like that should work.

For reference, i'm about half way through enhancing the error classes, so don't worry about those.

micah686 commented 7 years ago

So I was going to edit the SetVoteList to return the values needed, but it looks like you reduced the boilerplate code and changed a lot of things, so my changes wouldn't be applicable for this pull request. So I'm closing this pull request, because I doubt any change I made will fit with the new structure.

Nikey646 commented 7 years ago

I had placed that into a separate branch to ensure that there wouldn't be Merge conflicts in regards to this PR, but i believe that most of the changes i made to reduced the redundancy required for most commands may make this PR obsolete, as you mentioned.

I did, however use your implementation of the 'set' commands, just using the modifications i suggested. If you want, i can merge the changes into master, so you can work off of that version instead. In regards to completing the commands, it should just be defining the public methods and calling the internal method for set commands, using the newly created Constants class to provide the command text (set votelist and etc)

micah686 commented 7 years ago

You can merge them if you want, but I'll probably be working in the new branch. I'll probably re-implement the mono warning, if(HasMono != true) { Console.WriteLine("Mono and .NET Core's SecureString is NOT secure. Please consider an alternative, or warn your users"); } into the new version, since it'll probably be simpler that way.

Nikey646 commented 7 years ago

Alrighty, i'll leave it in a separate branch for now so i can ensure i didn't mess anything up before merging it (Like forgetting one of the constants... looks at the latest commit...whistles)