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 Default Values to Documentation Pages #32

Closed
swar opened this issue Jan 8, 2019 · 9 comments
Closed

Add Default Values to Documentation Pages #32

swar opened this issue Jan 8, 2019 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@swar
Copy link
Owner

swar commented Jan 8, 2019

Per #31 (comment)

I want to add Default Values to the documentation page so users do not need to fumble in figuring out what values they need to pass.

No timeline when I can finish this task, currently with a hand injury in my dominant hand so will need to wait a bit for this to heal up before I can commit time to this issue.

@swar swar added the enhancement New feature or request label Jan 8, 2019
@swar swar self-assigned this Jan 8, 2019
@swar
Copy link
Owner Author

swar commented Jan 29, 2019

Hand is starting to heal so I should be able to dedicate time into this issue.

@TK05
Copy link
Contributor

TK05 commented Apr 18, 2019

I think this is a possible solution: TK05@0211681

Which outputs this parameters table:

Parameters

API Parameter Name Python Parameter Variable Default Value Pattern Required Nullable
LeagueID league_id 00 (00)|(20)|(10) Y
Season season 2018-19 Y
SeriesID series_id_nullable Y

@swar
Copy link
Owner Author

swar commented Apr 18, 2019

The only issue with this is that on some pages the parameters table becomes unbearable to view. I tried completing this issue earlier, but I couldn't get it to fit in the page, so I scrapped what I had done.

Maybe we need to create another table of just Python Parameter Variable and Default Value instead?

@TK05
Copy link
Contributor

TK05 commented Apr 18, 2019

As it stands, default_py_value and parameter_value only really differ when parameter_value has a statically applied value to make for a valid url. This is true for ~ 40 parameters. I believe this key: value is only relevant for endpoint documentation so it might be best to store these "url helpers" elsewhere and revert parameter_value to their true defaults which would allow for them to be more easily accessed.

As for the table, is this because some of the patterns can be quite long? An option could be to just create another table like "Parameter Patterns". Might also be helpful to include known valid patters. Season seems to be a parameter with known patterns that isn't displayed for some reason (bug?). Also some nullable parameters have known patterns that could probably be helpful to display as well.

@swar
Copy link
Owner Author

swar commented Apr 18, 2019

I'm not sure what you mean by your first paragraph. Can you elaborate? What do you mean by "url helpers"?

It's mostly because the parameter patterns are very long for some endpoints. And then some of the default values might also be long. Adding another column doesn't flow well with GitHub.

Might also be helpful to include known valid patters. Season seems to be a parameter with known patterns that isn't displayed for some reason (bug?).

This is something I think we would need to append as User Added Information so that it's clearly stated in the endpoints. I think this information will eventually be easier to add and display in the future documentation website that is still pending.

Right now the best method of action of accessing parameters or information regarding a parameter is to click on the parameter and view the information in the paramater.md file, since that is all user defined information. (Example: SeasonType )

@TK05
Copy link
Contributor

TK05 commented Apr 18, 2019

parameter_value is used by get_query_string_parameters in the endpoint doc generator.py to generate a valid URL. It's also used a few times in analysis.py to essentially form a valid request to allow the analysis to continue.

Basically any parameter that has ID in the name (and is non-nullable) has a statically set value to allow for the generation of a valid URL. Exceptions are things like LeagueID where the possibilities are limited and set in parameters.py and SeasonID for similar reasons. Parameters like anything TeamID or PlayerID related need a value set otherwise they could never be analyzed because a Team or Player is required to generate a valid response.

parameter_value and default_py_value only differ when these statically set values are needed to generate a valid response. That's what I mean by "url helpers" as these values are only needed to help generate a valid URL. Otherwise, parameter_value and default_py_value are the same and only differ in type.

So if we moved those statically set "url helpers" (basically anything in parameter_variations with a comment attached) to a separate reference table, we could then more easily reference default values by just looking up parameter_value directly. Any parameter that previously had a statically set value to generate a valid response would now essentially have None as it's parameter_value which would be accurate because it doesn't have a default value.

As for the second part, I agree about having User Added Information. I've been thinking of some ideas on that for a few days. Maybe it's worth creating a separate issue to brainstorm on that one.

The Season parameter tho is maybe a good example of including known valid patterns.. You can compare the table #31 (comment) with the current markdown commonplayoffseries.md. The pattern for Season has disappeared likely because a more recent analysis of the endpoint showed that the response no longer returns those patterns. But we still use Season.default to generate a valid URL and response and thus we know an acceptable pattern for the Season parameter but don't show it in the endpoint doc. There's a few parameters like this that I have seen and a few nullable parameters like this that have valid patterns that aren't user supplied. That's what I meant by including known valid patterns.

@TK05
Copy link
Contributor

TK05 commented Apr 20, 2019

Maybe it'll make more sense just to see the refactor master...TK05:development/doc-default-values

Update: af88963 - Added default values to endpoint doc template and generator

Update 2: bc88f5a - Finalized changes w/ separate parameter patterns table. Example Endpoint Doc

@swar
Copy link
Owner Author

swar commented Apr 29, 2019

I am hesitant to switch parameter_value to be the default value used in the documentation because that field is directly necessary to use the other tools that are set up. As you mentioned it is used to create a valid endpoint URL and used to create valid requests when testing endpoints.

I think a better solution would be to eval the default_py_value from the nba_api/stats/library/parameters.py file, since that is where we explicitly declaring default values. We can ignore any field name with "Nullable" in the value, and then all default_py_values of None will need to be a required input by the user. Everything else should be coming from that parameters.py file.

Now this is not a great solution, but it's a bandaid to make things work without having to rework the tools. I would say if we were to rework the mapping file, we might as well rework most of the tools and switch templating over to use Jinja or something else that's more reliable than using literal python triple quote string formatting. I just don't want to make a major change into that process now, and then down the line, make another major change while reworking the functionality of the tools. I also think it would be best to wait until after the summer when the next NBA season is going to start to make a major change to the functionality of all tools. Just so we don't get blindsided with changes over the summer or prior to the start of the next season.


For User Added Information, these changes are going to change how we interact with the endpoint documentation creater tool. I think the best method is for us to store the additional information inside a JSON file to be rendered by the tool in the .md file. I don't see another better way of making these types of changes, because a JSON file can be used later in a documentation website as well (the future plan that I need to get on top of). And I would imagine it would be easier for developers/users to add information that way. Now the only issue is how and what it would interact with, which would need more example documentations to see how exactly we would want it to work.


As for as your example endpoint, I think this is going to be the best way of rendering default values inside those .md files. I like that a lot as it won't make the page extend too wide. And I think we should explictly flag None as Required Input or something along those lines so that end-users understand that this is a value that is required to be inputted. None can be confusing especially when we throw nullable values into the mix.

@TK05
Copy link
Contributor

TK05 commented Apr 29, 2019

I created a new issue for the User Added Information topic to get the ball rolling.

On the default values I agree that it makes more sense to reference parameters.py directly for the defaults since that is where they're pulled from.

I think refactoring the tools to adjust for any new style of mapping shouldn't be too hard. I'd be willing to help get them working again if you go a different direction with the mapping design. The tools should support the project and not necessarily the other way around so a few hours of work would get everything back in order no problem.

Switching to a template engine moving forward definitely makes sense and will help make any further additions to an endpoint documentation much easier.

We can hold off on any big changes until the off season but maybe a new branch for WIP features to test and get the ball rolling if nothing major changes for next season might be wise.

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

No branches or pull requests

3 participants