-
Notifications
You must be signed in to change notification settings - Fork 448
Adds max_age param to csv and pdf request options #530
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
Conversation
| params.append('orientation={0}'.format(self.orientation)) | ||
|
|
||
| if self.max_age: | ||
| params.append('maxAge={0}'.format(self.max_age)) |
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.
Should we do some verification that this is an integer at least?
| Landscape = "landscape" | ||
|
|
||
| def __init__(self, page_type=None, orientation=None): | ||
| def __init__(self, page_type=None, orientation=None, maxage=None): |
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.
Can we call it max_age here as well since it’s what you call it on the object anyway
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.
ImageRequestOptions already had maxage, so I wanted to keep it consistent across the 3 request objects without introducing any breaking changes. I guess we could deprecate it if we really wanted to change it?
|
oops i made the change in a different branch. |
| try: | ||
| return int(value) | ||
| except ValueError: | ||
| print("Integer value expected for the '{}' parameter, but got {}".format(name, 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.
I don't think we want to just print something in this case. We should throw an exception that tells what the error is. I believe we have something for Integer parameter errors
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 have property decorators in models for Integer parameter errors, but didn't want to bring that into here.
Above snippet prints a message and re-throws the same ValueError. It would be ideal to throw an exception with our custom message, but that results in 2 exceptions being thrown.
Max_age query parameter was recently added to the REST API endpoints for query view data/pdf and query workbook pdf. This PR adds max_age param to the csv and pdf request options.
It was already in place for view images.
Will make doc updates separately for that branch.