jozefhajnala / nhlapi

A Minimum-Dependency R Interface to the NHL API
GNU Affero General Public License v3.0
30 stars 7 forks source link

plot_rink() helper #4

Closed MichaelChirico closed 3 years ago

MichaelChirico commented 4 years ago

Following up this Twitter thread:

https://twitter.com/michael_chirico/status/1301906742679396355

I have most of the code for plot_rink() done using base plotting & assisted by the NHL rulebook:

https://nhl.bamcontent.com/images/assets/binary/308893668/binary-file/file.pdf

Advantage over raster being auto-scaling.

Shouldn't be too hard to come up with a ggplot_rink or ggrink but I'm terrible with ggplot2 so I would leave that to someone else.

Filing an initial implementation shortly...

jplecavalier commented 4 years ago

I was thinking about creating a package exclusively for drawing sport surfaces (ice hockey rink, soccer field, baseball field, football field, tennis court, etc.) in ggplot2. Actually, I saw that someone had the same idea a couple of years ago (https://github.com/woobe/ggsports) but for some reason, did not implement it. I really like the name ggsports though...

In my own opinion, I think it's probably better to keep this package about getting data from the API.

MichaelChirico commented 4 years ago

@jplecavalier I do think that makes sense as an end goal. In the mean time I'd rather have it available somewhere, and it can be refactored later once the momentum builds.

At present I only see 4 packages on CRAN related to hockey (searched for NHL and hockey in the package list):

So for the moment, I think it makes sense to build some "extraneous" functionality into this package (PR #5 closing this adds no dependencies, though a gg version would probably have to add a ggplot2 Suggested package) until a better home is up & running.

Ultimately up to the maintainer of course.

Definitely appreciate thinking about design in this way btw!

jplecavalier commented 4 years ago

I would see no problem of temporarily implementing it here except the breaking changes that would result later with a refactor. But still, that's probably acceptable for a 0.y.z version.

MichaelChirico commented 4 years ago

Yea with an early ecosystem like this we can do a deprecation cycle like (each step is a different nhlapi release)

0: other package (say nhl) is born 1: Add nhl as Suggested here and make plot_rink a re-export of that 2: Add a warning in nhlapi about pending deprecation 3: Deprecation in nhlapi (error on usage, and remove Suggested dependency) 4: Completely remove plot_rink from nhlapi

This is an extremely careful cycle and some steps could be eliminated

jplecavalier commented 4 years ago

That would work perfectly with me

jozefhajnala commented 4 years ago

Thank you for the discussion! Sorry for the late response, I think it is a good idea to have the rink within {nhlapi} for now. Let me have a look at the PR and get it in.