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

Allow state option to be overridden #2

Merged
merged 1 commit into from
Jun 8, 2016
Merged

Allow state option to be overridden #2

merged 1 commit into from
Jun 8, 2016

Conversation

cvinson
Copy link
Contributor

@cvinson cvinson commented Jun 5, 2016

Small change to avoid overwriting the passed in state param.

@analog-nico
Copy link
Owner

Hey @cvinson !

The changed code still always sets options.state to true. It should be options.state = options.state === false ? false : true.

However, the state parameter is intentionally set to true because this increases security. See https://developers.pinterest.com/docs/api/overview/ and search for "state". Why do you ever want to set it to false?

@analog-nico
Copy link
Owner

Or do you intend to pass your own state value? If yes, I am curious to know the use case! Thanks.

@analog-nico
Copy link
Owner

Any thoughts about its usage so that I have something I can add to the README?

@cvinson
Copy link
Contributor Author

cvinson commented Jun 8, 2016

You are correct. Not sure what I was thinking there. I'll make those changes here shortly.

Passing my own state values is exactly my use case.

@analog-nico
Copy link
Owner

No, sorry. Your code is correct for passing strings. Just false wouldn't be possible. But disabling the additional security is anyway not a good idea.

I'll take it from here. Thanks for your contribution!

@analog-nico analog-nico merged commit 64a7ee3 into analog-nico:master Jun 8, 2016
@analog-nico
Copy link
Owner

I just released version 0.4.0 which allows string values for options.state.

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.

None yet

2 participants