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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add examples README.md #1238

Merged
merged 1 commit into from Nov 14, 2014
Merged

Add examples README.md #1238

merged 1 commit into from Nov 14, 2014

Conversation

simov
Copy link
Member

@simov simov commented Nov 1, 2014

Related to #1202 - I decided not to write tests, because the examples I just added would require apps, credentials, as well as storing refresh tokens.

The first two examples are just for refreshing tokens, the OAuth1.0 one I'm using only for Yahoo ATM, the OAuth2 one seems much more widely adopted.

About the Flickr example, there are modules out there just to solve that problem, so I though it would be useful to share it.

Also I have no idea if that was the point with the examples, so feel free to scratch it out 馃榾

@FredKSchott
Copy link
Contributor

awesome! The response callbacks could be cleaned up a bit though (uncommenting the asserts, adding some more asserts, adding some error handling, etc).

@simov
Copy link
Member Author

simov commented Nov 2, 2014

No problem, I actually have all of this tested here https://github.com/simov/purest/tree/master/test/request but for the purpose of this example it will be too much of a noise to add everything needed, just to get a real system test going.

So in this case the examples are build on trust 馃榾

@nylen
Copy link
Member

nylen commented Nov 5, 2014

What I had in mind is more a README that links to example files, then each example file is a .js file which ideally can be run without any modification. I think we could eventually have a lot of different examples, and that would make it easier to add new ones.

Others feel free to comment or disagree though. I agree that for OAuth it's probably too much trouble to get fully working examples, but I do have a couple that work at https://gist.github.com/nylen/639ef899d9964cb2957c (created for testing #1228).

@simov
Copy link
Member Author

simov commented Nov 5, 2014

Yep I saw your tests regarding OAuth, most of the stuff can be done that way. My point was that some times the returned JSON response from the provider doesn't indicate whether the result is successful or not. I mean it can return success but still the uploaded image to be corrupted for example. So in such cases additional requests are needed that further deteriorate from the example. Plus some tasks are queued and need some random time before they can be checked for.

Otherwise I completely agree with that, I had something similar in mind regarding the tests, still thinking about it though.

@nylen
Copy link
Member

nylen commented Nov 5, 2014

I agree, I don't think we want to do a bunch of extra work to automate testing the examples. If we can easily get a non-zero exit code with something like if (err) throw err; then great, but the examples are more just to show "here's what is possible with this library".

@FredKSchott
Copy link
Contributor

Also just to clarify, the code itself looks great. What I originally meant by "cleaning up the callbacks" was that you want to keep examples simple, and right now all the request callbacks seem to differ from one another. A commented out assert will probably lead to more confusion than anything.

Unless you want to build a full example with an example callback, I'd recommend just adding comments, or some simple error handling and console logging to each one. Let the reader decide what they want to do with the response.

For example:

}, function (err, res, body) {
  // if err is not null, an error occurred
  // otherwise, body contains the response body
}

This is all just personal preference / bikeshedding btw, +1 regardless :)

@FredKSchott
Copy link
Contributor

Lets merge this as is, I'd rather not block people from contributing (@tbuchok was asking about helping in #1202). We can always change / refactor how we store these later if the README doesn't scale well.

FredKSchott added a commit that referenced this pull request Nov 14, 2014
@FredKSchott FredKSchott merged commit 8e3aa3b into request:master Nov 14, 2014
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

3 participants