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

2 more query functions #8

Closed
wants to merge 3 commits into from
Closed

2 more query functions #8

wants to merge 3 commits into from

Conversation

Fulnir
Copy link
Contributor

@Fulnir Fulnir commented Mar 9, 2018

query(conn, "anyofterms", "name", "VI")

query(
        conn,
        "anyofterms",
        "name",
        "VI",
        "uid name release_date starring { name }"
      )

removed: import_starwars_sample() from mutation_test setup (double mutation)

query(conn, "anyofterms", "name", "VI")
query(
        conn,
        "anyofterms",
        "name",
        "VI",
        "uid name release_date starring { name }"
      )
removed: import_starwars_sample() from mutation_test setup (double mutation)
@coveralls
Copy link

coveralls commented Mar 9, 2018

Pull Request Test Coverage Report for Build 79

  • 36 of 38 (94.74%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+4.02%) to 78.534%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/exdgraph/query_builder.ex 26 28 92.86%
Totals Coverage Status
Change from base Build 78: 4.02%
Covered Lines: 150
Relevant Lines: 191

💛 - Coveralls

@ospaarmann
Copy link
Owner

Hey,

I am not sure if this is the way to go since queries can become pretty complex. I think we should first discuss a good way to model GraphQL+ queries in Elixir before going down a specific route or implementing specific query functions for certain types of GraphQL+ functions.

Take this example from Manish Rai Jains talk

@Fulnir
Copy link
Contributor Author

Fulnir commented Mar 13, 2018

I think it's good to have a few levels of access.

the graphql level
query(conn, "{ starwars(func: anyofterms(name, "VI")) { uid name starring{ name } } }")

some quick and easy queries
query(conn, "anyofterms", "name", "VI")

and a query builder like this draft concept

    graph_query = GraphQL.graph_query(conn, "starwars")
    graph_query
    |> query_root(:allofterms, {"name", "Star Wars"})
    |> first(2)
    |> filter(:has, "genre")
    |> filter(:or, :le, {"production-cost", "20000000"})
    |> order(:desc, "initial_release_date")
    |> return(["uid")
    |> return(["name", "initial_release_date"])
    |> return_expand("director.film")
    |> filter_expand(:le, {initial_release_date, "2000"})
    |> return_expand(["name@en", "initial_release_date"])
    |> query()

@ospaarmann
Copy link
Owner

So I had a look at your PR again. Thank you! I think having more query functions is in general a good idea. But there are a couple of issues.

  • There are still IO.inspect calls in the code
  • Your implementation puts object always in quotes, assuming that it is always a string. This will not work with some functions (eq(), gt(), lt(), etc.)
  • The display parameter should have a default value
  • It is not possible to select multiple predicates that should be returned (if I'm not mistaken)
  • Some query functions don't have an arity of 2, for example uid()
  • It doesn't support pagination like me(func: has(director.film), first: 5)
  • It doesn't support sorting

This might seem like much. But I don't want people to start using a function and then suddenly change the API and break their application. So I would rather try to go for a more complete integration.

What do you think?

@ospaarmann
Copy link
Owner

I think a good way to handle this is this client: https://github.com/elbow-jason/dgraph_ex

@ospaarmann
Copy link
Owner

And @Fulnir can we please have a discussion on here first before you submit a PR?

@Fulnir
Copy link
Contributor Author

Fulnir commented Mar 17, 2018

To avoid new commits appearing here, I closed the pull request.

@Fulnir Fulnir closed this Mar 17, 2018
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