Skip to content

Schema function additions #6

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

Closed
wants to merge 10 commits into from
Closed

Conversation

schloerke
Copy link
Contributor

To keep in the spirit of libgraphqlparser (speed), I added a second function schema2json(txt) that allows for both queries and schema definitions while still keeping the original 'query only' function graphql2json(txt). (link) My opinion is to only use the schema and query parsing function, but I see the point of a small speed increase for query only parsing.

Feel free to change the name to something else. :-), just exposing the required functionality.

I also upgraded the tests for kitchen sink queries and schema definitions.

Side note...
This update is so nice for me! It saves me about a >1000 lines of code for a small to midsize schema definition. I will now be able to do something similar to https://github.com/matthewmueller/graph.ql

@jeroen
Copy link
Member

jeroen commented Jan 11, 2017

Thanks! Perhaps we can avoid duplicating a lot of C code binding code by creating a single c++ function with a parameter that specifies which parser to use?

// [[Rcpp::export]]
Rcpp::String dump_json_ast(Rcpp::String graph, bool schema) {
  graph.set_encoding(CE_UTF8);
  const char *error;
  auto AST = schema ? 
    facebook::graphql::parseStringWithExperimentalSchemaSupport(graph.get_cstring(), &error) :
    facebook::graphql::parseString(graph.get_cstring(), &error);
  if (!AST) {
    char buf[1000];
    strncpy(buf, error, 1000);
    free((void*) error);
    throw std::runtime_error(buf);
  }
  const char *json = graphql_ast_to_json((const struct GraphQLAstNode *)AST.get());
  Rcpp::String out(json);
  out.set_encoding(CE_UTF8);
  free((void*) json);
  return out;
}

Would that make sense?

@schloerke
Copy link
Contributor Author

schloerke commented Jan 11, 2017

Sounds great to me! Plus it's much less confusing

From the "speed" of R standpoint, I would enable schema parsing by default.

@schloerke
Copy link
Contributor Author

Oops, I made a bad example. Forgot to use the new function... but that'll get fixed with the merging of the functionality

@schloerke
Copy link
Contributor Author

Merged into one function now. Tests updated. Added schema example to graphql2json() fn.

@jeroen
Copy link
Member

jeroen commented Jan 11, 2017

OK looks good. Can you add yourself as a contributor to DESCRIPTION? Also would you mind rebasing to squash the commits into a single commit?

@schloerke schloerke mentioned this pull request Jan 11, 2017
@schloerke
Copy link
Contributor Author

squashed in #7

@schloerke schloerke closed this Jan 11, 2017
@jeroen
Copy link
Member

jeroen commented Jan 11, 2017

Thanks. Next time you don't have to do a new PR, just git push -f :)

@schloerke
Copy link
Contributor Author

Ah! I've usually just done with within the Github PR (but can't cuz I'm not owner here), didn't know how to do it command line side. Thank you!

@jeroen
Copy link
Member

jeroen commented Jan 11, 2017

I always have to look it up but it's really easy: https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/

@jeroen
Copy link
Member

jeroen commented Jan 12, 2017

This is on CRAN now.

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.

2 participants