Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dynamic Plot Endpoints #897
Dynamic Plot Endpoints #897
Changes from all commits
77901fd
4c9a87a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's let the endpoint determine it's serializer params.
This will allow for the endpoint to have full control vs using a globally set flag.
Counter example app: API with two routes: 1 w/ dynamic serializer and 1 w/ static serializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I don't like it, let's make the camelCase and change to
dynamicSerializerParams
(or something to hint that it is the dynamic serializer params...dynSerParams
?)Will need to change all usage as well. :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a
rlang::missing_arg()
as a placeholder and handle it if it is missing via the option value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use a
rlang::missing_arg()
as a placeholder and handle it if it is missing via the option value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding validations on
dev_on()
** Untested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the immediate feedback...
Given how much there is, it might be an idea to make and edit these changes in an editor and to add some unit tests as we go?
If it helps, we can hold off on this (and the other PRs) until RStudio have capacity to invest the time? (That might also help me negotiate some wriggle room with my work pressures too!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling the
serializer_params
** Untested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Using larger name from above suggestion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might work?
** Untested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching cosmetic change below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same change as above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your motivation to use
plot_*
for the arg names?Why not pass them through directly?
My interpretation:
plot_*
@serializer png list(width=200)
http://....../plot_route?plot_width=200
If there isn't too much motivation for using
plot_*
names, I'd like to keep the consistent arg names when possible. (Knowing there could be param name clashing with the regular route params)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well as avoiding clashes, my main motivation behind
plot_
names was to make it easier to distinguish the plot parameters from the business logic - e.g. in https://.../palmerPenguins?min_flipper_length=200&min_bill_length=40&plot_width=400&plot_height=500The naming also makes the business logic vs plot parameter separation obvious in the Swagger UI (maybe some more ordering there might also help?)
e.g. it's obvious here which params are business vs which are plot:
Asides:
plot.width
to try to make the plot parameters look like a separate object in the REST url (but couldn't spot any docs on that in https://swagger.io/docs/specification/describing-parameters/)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cosmetic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should allow users to determine if the dynamic params are used as the serializer level.
Maybe something like:
Will need to document the
dynamic_params
parameter.