search

beneggett / sportradar-api

19 stars 13 forks source link

Setup & Contribution #19

Closed trevorrjohn closed 7 years ago

trevorrjohn commented 7 years ago

Hey there, my team and I are going to be implementing golf with either this gem or sports_data_api, which is the gem we have used in the past. However it seems like this gem is being actively worked on. I forked it the other day to start to work on it, but ran into a few stumbling blocks which raises some concerns for us.

First, it seems like there were some api keys committed in the VCR cassettes. Second, api keys are missing from the .env.sample file so as someone who is new to the project I had a hard time trying to figure out what I needed in my .env file. It would be very helpful to keep this file updated for new contributors. Third, some of the tests don't actually seem to match the name of the test (see: live_images_test.rb#L21). I am just concerned that there are lot of cassettes that don't seem valid yet they are used in tests. Can someone explain the thought process here? Lastly, I am a little concerned with all the puts statements committed around the codebase and I don't necessarily think that this is a good practice.

Let me know if you have any questions about my concerns. I would love to contribute, I just need to be sure that you all are open to my contributions.

Thanks!

phoffer commented 7 years ago

Trevor, we would love to collaborate with you. We are in rapid development right now, and would love some extra help.

Regarding your concerns:

  1. The cassettes with keys have been removed (EPL Image key is dead, future tests won't include it).
  2. As for the environment variables, there were a few missing (soccer and college football). I have added those. As for actual values, you will need to obtain trial keys from Sportradar. They don't have any public trial keys AFAIK.
  3. Some of the tests should be renamed-- the images example one likely says manifest because that's the terminology SR uses. We are working to improve things as we've gotten a better idea of our needs for the gem and the interfaces we need.
  4. Agreed about puts statements. I see 7 in the gem, and they can all be removed as they were just useful in development. We've been moving quickly and are just starting to come back to improvements.
beneggett commented 7 years ago

Hey @trevorrjohn, we'd welcome your contributions, especially in sports we have yet to implement. As you might guess, we checked out the sports_data_api gem as well and found it, for our use case, unfortunately outdated - especially as NFL switched to the official feed this year, which was quite different than the previous version, and support for soccer was missing & those were the seasons we were first developing against.

Our design strategy is to first expose the api and be able access it's data in a meaningful way through POROs. We're learning more and more about the nuances of their data models & are trying to normalize what can & should be normalized into patterns, etc.

@phoffer can give you a bit of insight into our sports development schedule & strategy as well.

We certainly welcome suggestions (especially those associated with PRs) and happy to discuss ideas prior to.

beneggett commented 7 years ago

Closing the Github issue. Feel free to revisit anytime