peterborah / vigor Goto Github PK

This should be simple, but I haven't gotten around to it yet. We should switch to one class per file.

(I'm going to start adding issues for anything I need to do, but am not immediately working on. Feel free to beat me to the punch with a pull request!)

We need to handle non-200 responses. This should be a centralized thing, rather than something that each method that makes an API call handles itself. I'm in favor of raising exceptions, as a general pattern, but I don't have a design in mind yet.

We will still iterate on the best design for the stuff we've implemented, but we should try to hit 100% coverage of the API as soon as we can. To do this, we need to come up with intuitive models for the various domain objects.

In particular, statistics are both crucial and modeled (in my opinion) in an ugly way. Suggestions for a useful and intuitive interface welcome!

A side effect of 5d49f78 is that it's now possible to ask Vigor to do things before properly configuring it. When you do so, it blows up inside Httparty. It should instead give a meaningful error.

Sort of a "nice to have", but configuring a CI like Travis-CI to run our tests after pushes would be pretty cool.

We should probably do something like create a test summoner that we don't mess with too often, so that we can rely on it having the same data. Bumping to the latest version of the API was unnecessarily painful for @zetsubo, since he had to change the tests to reflect new games I had played...

Today, when writing the rune test, I found myself wanting to type:

rune_page.select{...}

Why not? RunePages are basically just collections of runes, with a couple other details. Maybe we should make them act like collections.

Sort of a nuisance trying to find out who played what champ in a game because a game is returning the champion's id, not their name. I'm not sure if you already support finding a champion by its id, but here's what I suggest.

def self.champion(lookup_value)
if lookup_value.is_a? String
self.all_champions.find {|champ| champ.name == lookup_value}
else
self.all_champions.find {|champ| champ.id == lookup_value}
end
end

P.S. I'm pretty new to all of this stuff so if this was just a waste of time, sorry :(
Edit: I have no clue how to use github's flavored markdown so it looks all bad

We have real Champion objects, so let's use them! Calling .champion on a game/summoner/etc. should retrieve the full Champion object from the ID.

It would be nice to have the base url be configurable or be able to switch between regions :-) Even though one could most likely just call Client::Vigor.base_url = "different url", having it pleasantly wrapped would be nice imho :-)

Hi,
Great job with the wrapper. I've been using your work and I found a strange error. This could just be my client, but something weird happens when I look up the recent games of "boy54"

 summoner = Vigor.summoner 'boy54'
=> #<Vigor::Summoner:0x2704bc0 @fields={:name=>"boy54", :profile_icon_id=>589, :level=>30, :revision_date=>#<DateTime: 45972-11-07T02:56:40+00:00 ((18512299j,10600s,0n),+0s,2299161j)>}, @id=43215051>

recent_games = summoner.recent_games
NoMethodError: undefined method `map' for nil:NilClass
    from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/vigor-0.5.0/lib/vigor/game.rb:8:in `initialize'
    from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/vigor-0.5.0/lib/vigor.rb:49:in `new'
    from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/vigor-0.5.0/lib/vigor.rb:49:in `block in recent_games'
    from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/vigor-0.5.0/lib/vigor.rb:49:in `map'
    from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/vigor-0.5.0/lib/vigor.rb:49:in `recent_games'
    from C:/RailsInstaller/Ruby1.9.3/lib/ruby/gems/1.9.1/gems/vigor-0.5.0/lib/vigor/summoner.rb:47:in `recent_games'
    from (irb):94
    from C:/RailsInstaller/Ruby1.9.3/bin/irb:12:in `<top (required)>'
    from -e:1:in `load'
    from -e:1:in `<main>'

Just about every other summoner I can think of works fine. Think you can take a look ? Since game data might change before this is reproducible, I did a pastebin of the output from developers.riotgames, in case it helps. http://pastebin.com/bdQRscQ9.

https://developer.riotgames.com/discussion/riot-games-api/show/rXIx7RPr

There's improvements to all endpoints except Champion now - ones that jump out immediately at me are that masteries are now returned sorted, and empty pages now return an empty object. We should start updating endpoints to v1.2/2.2 and changing code to reflect changes.

All the tests are currently in two files. While it's true that those two classes contain most of the complexity, we should probably organize things in a better way.

Riot's Game API is designed such that it lists information for the player you searched for on the game object itself, and then leaves that player out of fellowPlayers. Should we hide this fact, and just embed the searched-for player in the list of players?

This is a pretty broad one, and will probably be an ongoing concern:

Our library should be smart about when it makes API calls. We don't want to design it in a way that encourages people to hit Riot's servers every time they want to look at a piece of information. I'm not sure what a unified solution would look like, but it's something to keep in mind.

We should have at least few tests that actually check the connection to Riot's servers, but we shouldn't hit the servers for every test. The current pattern is unsustainable, because of rate limiting, and also just the slowness of making API calls with every test. Also, I don't want to break the test by changing my masteries. :P

We should investigate gems like webmock and vcr, and find a sustainable pattern that still gives us realistic tests.

Once we get full coverage of the API, the next big milestone is a 1.0.0 release. In my mind, that means that we commit to maintaining backwards compatibility (at least until 2.0.0), and that we feel it's ready for production use.

I think we need at least:

• Full API coverage
• A solid caching scheme, or a decision to avoid caching for real
• Syntax that we're happy with, for everything

Anything else?

At the moment, the natural path for getting to someone's masteries (or runes, or in the future other things) is to instantiate a Summoner object with vigor.summoner(id), and then call .mastery_pages on it. This will create two API calls, regardless of whether you care about the summoner information. There should be a straightforward, intuitive way to get masteries without having to to load the summoner first.

I got an

undefined method `map' for nil:NilClass

when trying to list all recent game for my summoner but I saw that you don't check wether it was a bot game or with other player.

This issue is that when the game include only bot, the hash fellowPlayers is empty and so the line

@fellow_players = data["fellowPlayers"].map { |player| Summoner.new(player, :game) }

create the exception.

I'm very new to ruby but thanks to RubyMine I was able to fix this issue on my side :

if !data["fellowPlayers"].nil?
    @fellow_players = data["fellowPlayers"].map { |player| Summoner.new(player, :game) }
end

but there may be a better solution for that

I got lazy and left Game.stats as a hash. This probably violates the principle of least surprise.

It just returns the games in some random order (I'm not too sure how it's being given back). However, this code would return the games in order from the most recent to the least recent which would be more convenient.

def recent_games
    games = Vigor.recent_games(@id)
    games.sort { |x, y| y.created_at <=> x.created_at } 
end

Edit: Phew finally got the spaceship operator to show up