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

Add GraphQL variables #291

Merged
merged 11 commits into from
Aug 5, 2019
Merged

Conversation

joelvh
Copy link
Contributor

@joelvh joelvh commented Jul 5, 2019

This PR allows specifying GraphQL variables to the request. Keeps backward-compatibility.

@coveralls
Copy link

coveralls commented Jul 5, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 85668e3 on joelvh:feature/graphql_variables into bc6f1f9 on MONEI:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0d66e4f on joelvh:feature/graphql_variables into 1cf1817 on MONEI:master.

@joelvh joelvh force-pushed the feature/graphql_variables branch from 88f17a7 to 93fe6c0 Compare July 5, 2019 06:46
@joelvh
Copy link
Contributor Author

joelvh commented Jul 5, 2019

@lpinca not sure if you're the primary guy, but wanted to see if you know why the coverage test is failing?

@lpinca
Copy link
Collaborator

lpinca commented Jul 6, 2019

probably because the additional if branch you added is not covered by a test.

@alexandresaiz
Copy link
Contributor

@lpinca hey luigi. cana you decide on this PR?

@lpinca
Copy link
Collaborator

lpinca commented Jul 29, 2019

I will not review it until checks are green.

@lpinca
Copy link
Collaborator

lpinca commented Aug 1, 2019

FWIW I'm 👍 on the feature. I suggested it when GraphQL support was added. See #282 (comment).

@joelvh
Copy link
Contributor Author

joelvh commented Aug 1, 2019

Got tests passing.

@joelvh
Copy link
Contributor Author

joelvh commented Aug 1, 2019

@lpinca merge?

@lpinca
Copy link
Collaborator

lpinca commented Aug 2, 2019

Thanks, I will take a look when a I can.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

To keep the limit of 80 chars per line and be consistent with other test descriptions.

test/shopify.test.js Outdated Show resolved Hide resolved
test/shopify.test.js Outdated Show resolved Hide resolved
test/shopify.test.js Outdated Show resolved Hide resolved
test/shopify.test.js Outdated Show resolved Hide resolved
@joelvh
Copy link
Contributor Author

joelvh commented Aug 5, 2019

@lpinca resolved conflicts - your updates should all be there.

@joelvh
Copy link
Contributor Author

joelvh commented Aug 5, 2019

Now they should be there.

@lpinca lpinca merged commit 0a8d7c3 into MONEI:master Aug 5, 2019
@lpinca
Copy link
Collaborator

lpinca commented Aug 5, 2019

Thank you.

@joelvh
Copy link
Contributor Author

joelvh commented Aug 5, 2019

Yeah, something happened with the merge conflicts :-/ All good now. Thanks for approving!

@joelvh joelvh deleted the feature/graphql_variables branch August 5, 2019 15:56
@danalexilewis
Copy link

Just want to express my gratitude for this code - exactly what I needed! It will tidy up my string literals and also enable me to use types in my code! 🙏

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.

5 participants