Skip to content
This repository has been archived by the owner on Jan 3, 2022. It is now read-only.

add limitResponseLength caveat #95

Merged
merged 2 commits into from
May 22, 2020
Merged

add limitResponseLength caveat #95

merged 2 commits into from
May 22, 2020

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented May 22, 2020 •
edited
Loading

Adds a limitResponse caveat that slices an array response to the specified length.

@brad-decker brad-decker requested a review from rekmarks May 22, 2020 17:15
rekmarks
rekmarks suggested changes May 22, 2020
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given linting shenanigans, we should probably merge #96 first.

The caveat looks good though!

@brad-decker brad-decker force-pushed the limit-response-caveat branch from e0f1963 to d0f8352 Compare May 22, 2020 18:40
@brad-decker brad-decker requested a review from rekmarks May 22, 2020 20:08
@brad-decker brad-decker force-pushed the limit-response-caveat branch from d0f8352 to f36220a Compare May 22, 2020 20:17
rekmarks
rekmarks suggested changes May 22, 2020
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to suggest we make the name more specific by changing it to limitResponseLength, but then this is good to go!

@brad-decker
Copy link
Contributor Author

@rekmarks - done

@brad-decker
Copy link
Contributor Author

p.s Github might not be showing there's another commit, but there is. 👀

@brad-decker brad-decker force-pushed the limit-response-caveat branch from 3a53f66 to 4decc55 Compare May 22, 2020 21:18
@brad-decker
Copy link
Contributor Author

Fixed with a rebase, change is now actually there

rekmarks
rekmarks approved these changes May 22, 2020
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

I think we need to introduce more extensive caveat validation (ala PropTypes, because do we really want a limitResponseLength value of 0 or -1?), but that'll be its own PR.

@brad-decker brad-decker changed the title add limitResponse caveat add limitResponseLength caveat May 22, 2020
@brad-decker brad-decker merged commit 165fb4b into master May 22, 2020
@brad-decker brad-decker deleted the limit-response-caveat branch May 22, 2020 22:00
@whymarrh whymarrh mentioned this pull request May 26, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rekmarks rekmarks rekmarks approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brad-decker @rekmarks