PeterBorah / vigor

An unofficial ruby wrapper for the League of Legends API
MIT License
4 stars 4 forks source link

fixes #22, fixes #18 #29

Closed imogenkinsman closed 10 years ago

imogenkinsman commented 10 years ago
PeterBorah commented 10 years ago

Sorry I haven't had time to really look at this in the past couple days. Been traveling/seeing family. I have a few more days off work, and will not be traveling after tomorrow, so I should be able to put some more time into this very soon.

Thanks for continuing to do so much work on this!

imogenkinsman commented 10 years ago

No problem (and no rush!), I've been busy with family stuff too. I just got back home, so I'll (hopefully) put more time into this in the next few days.

I'm going to make another commit to this request in a bit.

PeterBorah commented 10 years ago

This looks good! I'm going to merge it, but before I release it as a new version of the gem I want to take a stab at coming up with a consistent caching scheme. As it is, there's no good way to grab the newest version of the champion data.

Super minor comment: In the future, would you mind having commit messages that said what they did, rather than just referencing the issue? It'll just make our lives a bit easier in the future as we're looking through the git log and such. Like I said, it's a super minor point, and it looks great otherwise!

imogenkinsman commented 10 years ago

Yeah, I was doing that because github links it and the issue that way... but it certainly makes the log hard to read. I think it might work better to make a descriptive commit message in addition to that.

Feel free to leave comments on my commits etc, I'm pretty thick skinned and I haven't done much ruby professionally, so suggestions and criticism are appeciated.

PeterBorah commented 10 years ago

Yeah, I think I prefer doing both. Like:

Find champions by ID (fixes #22)

And yeah, I'll try to do that when I see something. But I think you overestimate how experienced I am. ;) Feel free to make comments on my work too!