Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SynergyPlayTypes Endpoint #60

Merged
merged 6 commits into from
Apr 16, 2019
Merged

Add SynergyPlayTypes Endpoint #60

merged 6 commits into from
Apr 16, 2019

Conversation

TK05
Copy link
Contributor

@TK05 TK05 commented Apr 15, 2019

Added SynergyPlayTypes endpoint

  1. Added necessary parameter mappings. Also included known patterns for nullable parameters as base valid URL doesn't actually return any useful data. The added parameters patters will be useful for documentation purposes.
  2. Added endpoint md, updated parameters.md and analysis.json, added endpoint to tests.

@swar
Copy link
Owner

swar commented Apr 15, 2019

I'm in disbelief that someone else figured out how to add an endpoint. I'll check this out tonight and add it in once it passes my tests!

@TK05
Copy link
Contributor Author

TK05 commented Apr 15, 2019

It certainly took quite a bit of trial and error but it was a nice exercise to understand how the project is structured. Everything should be good but a few of the parameter patterns were specified by me which admittedly isn't ideal.

It's ok if this PR ultimately isn't committed because I know you can add this endpoint in a few minutes. I've already begun adding some functionality to the endpoint analysis tools as I think it would be beneficial to be able to generate and test user specified endpoints without having to potentially check all endpoints. Also working on a way to intuitively add user specified parameters and patterns to endpoints because I think that would be beneficial to endpoint docs.

SynergyPlayTypes for instance doesn't return any worthwhile data without PlayType and TypeGrouping parameters. The endpoint is valid with those nullable but it doesn't make for worthwhile documentation to leave the possible patterns out. Maybe this is a unique case but it still might be valuable to be able to have the ability to manually add specified data to analysis.json (or another json file) that can then be used by the endpoint tools to generate the appropriate files.

@swar
Copy link
Owner

swar commented Apr 15, 2019

My idea behind the analysis.json is to have only the data that is spit out by the endpoints as opposed to manual changes. So I may update that to leave out the manual changes you made. I would want us to map out everything in the parameters.py file instead.

If anything, I will merge your PR and fix up anything that needs to be fixed. This is so you can still get credit for the work that you have done. I am honestly impressed with how much you were able to get through despite the lack of documentation on how to add an endpoint.

@TK05
Copy link
Contributor Author

TK05 commented Apr 15, 2019

I agree it doesn't make much sense to have anything manually added to analysis.json. I did leave that commit for last 8821435. That commit does include fixing a typo in synergyplaytypes.md. It might be worthwhile to keep the additional patterns in the docs for now even if it means possibly being overwritten in the future but that's up to you.

I thought about adding a bit of documentation and comments to assist on endpoint analysis and generation but I'm not sure how many new endpoints will be added or how often they'll really need to be updated that way. The mappings.py file is still a bit of a conundrum to me but overall I didn't have much trouble following along with your thought process and design so kudos to you.

Great project and I look forward to many more commits.

@swar
Copy link
Owner

swar commented Apr 16, 2019

Thank you!

One last comment on the regex patterns is that I want the analysis.json and endpoint documentations to strictly be information returned to us. I know we can create our own regex patterns, but that might be better as a created tool so that we can control the accuracy and organization of it. In the endpoint docs, it should lead us to each parameter documentation which is built off of parameters.py so that will contain the most up to date information on the parameters.

I do think that the mapping file might need some overhauling maybe even a conversion to use json instead? Not sure, but I would need to brainstorm a better way to handle that.

@swar swar merged commit 8821435 into swar:master Apr 16, 2019
@TK05 TK05 deleted the develop/synergy branch April 16, 2019 02:23
@TK05
Copy link
Contributor Author

TK05 commented Apr 16, 2019

json would be a good idea imo. I'm not sure how you generate the parameters on your end but the flow kind of makes sense when you analyze it and get a few helpful errors along the way. Figuring out the intended order is a bit confusing which a json file would help with for better or worse.

One last comment on this PR, since we're going with the strictly information returned approach, it might be best to get rid of this line:

'^(Transition)|(Isolation)|(PRBallHandler)|(PRRollman)|(Postup)|(Spotup)|(Handoff)|(Cut)|(OffScreen)|(OffRebound)|(Misc)$': 'PlayTypeNullable'

You already cleaned up the leftovers from TypeGrouping and this was just an attempt from me to make sense of mapping.py.

I think leaving the parameters in parameters.py is a good idea and provides utility. Unfortunately there's nothing in the docs that speaks to this functionality which I suppose is a nice opportunity to add some functionality.... which I'll probably be working on this week. 👍

@swar
Copy link
Owner

swar commented Apr 16, 2019

Feel free to make another PR with that request so you can claim credit. I forgot about deleting that one. If not, I will go ahead and do that in the next few days.

In addition, it'll probably be helpful for me to make some documentation on how to add an endpoint. To be fair, I never anticipated anyone adding one themselves until you did it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants