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

Twitter API proposal #4

Merged
merged 42 commits into from
Jan 18, 2016
Merged

Twitter API proposal #4

merged 42 commits into from
Jan 18, 2016

Conversation

mr-wildcard
Copy link
Contributor

This is a simple schema for Twitter API. As this API is quite vast, I only implemented three types: User, Tweet, Retweet with few available fields. I also added a search query with 3 parameters.

Very simple tests have been provided for Twitter API, but I plan to add tests for twitter schema following graphql-js tests.

@clayallsopp
Copy link
Owner

This is awesome! Thank you so much for taking the time to come up with this, will leave some notes, but overall very excited

It looks like Twit is yelling in the Travis builds yelling about missing environment variables; I think you should be able to add some fake ones to .travis.yml

@@ -1,3 +1,157 @@
node_modules/
dist/
### WebStorm ###
Copy link
Owner

Choose a reason for hiding this comment

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

where did you pull this .gitignore from? My intuition is to use the github Node one, which covers more Node-specific things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for this brutal change. I pulled it from gitignore.io to cover most of the files generated from Webstorm and other IDE's that contributors may use. This gitignore also contains Node's common ignore rules. But I could merge Webstorm and Node specific rules if you wish, tell me.

Copy link
Owner

Choose a reason for hiding this comment

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

gotcha, that makes sense, can you add back in the line about "# Created by https://www.gitignore.io/api/webstorm,node" (which gives a bit more context/guides future users on how to edit it)

name: 'TwitterAPI',
description: 'The Twitter API',
fields: {
user: {
Copy link
Owner

Choose a reason for hiding this comment

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

I might prefer splitting this up into two fields - userById and userByScreenName, each with one GraphQLNonNull arg (is there a way in the GraphQL.JS API to have "mutually exclusive" field arguments?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

👍 good call

@clayallsopp
Copy link
Owner

@mr-wildcard anything else you want to do with this or good-to-merge? LGTM as-is

@mr-wildcard
Copy link
Contributor Author

hey @clayallsopp, if it's ok for you then it's ok for me :) let's :shipit:

clayallsopp added a commit that referenced this pull request Jan 18, 2016
@clayallsopp clayallsopp merged commit 1c8d2b4 into clayallsopp:master Jan 18, 2016
@clayallsopp
Copy link
Owner

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