Skip to content

Conversation

@cemnura
Copy link
Contributor

@cemnura cemnura commented Apr 29, 2020

Signed-off-by: Cem Nura [email protected]

@cemnura
Copy link
Contributor Author

cemnura commented Apr 29, 2020

@phillip-kruger is aware that I am working on the documentation for quarkus graphql extension that he is currently working on.

Comment on lines 102 to 106
public String name;
public Double height;
public Integer mass;
public Boolean isDarkSide;
public LightSaber lightSaber;
Copy link
Member

Choose a reason for hiding this comment

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

You might need getters and setters. I never tested public fields, but it might work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add setter and getters or I can add a comment line stating the setters and getters are omitted for brevity. However, that way make the documentation misleading.

I will try with public fields to see if it works and then will add setters and getters depending on that. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using public fields in the quickstart but got errors when omitting setter and getter methods. If I have understand correctly setter and getter methods must be included
in order to qualify as a field in GraphQL API.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, The spec is also not that clear on fields (microprofile/microprofile-graphql#226) and we are discussing it. We can see if we can build support for that in SmallRye. I'll add a ticket.

----

The server will return the complete schema of the `graphQL` API.

Copy link
Member

@phillip-kruger phillip-kruger May 7, 2020

Choose a reason for hiding this comment

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

You can maybe mention that in dev and test (and by configuration, even prod) Graphiql (UI) will be available in http://localhost:8080/graphql-ui

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okey, sounds like a great idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned Graphiql in the documentation. Great feature 👍

@phillip-kruger
Copy link
Member

This goes with the extension: #9144

@cemnura cemnura force-pushed the feature/graphql_docs branch from 9d18994 to 1c996a2 Compare May 7, 2020 22:31
Copy link
Contributor Author

@cemnura cemnura left a comment

Choose a reason for hiding this comment

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

@machi1990 @phillip-kruger Added the conclusion 🙂

-DprojectGroupId=org.acme \
-DprojectArtifactId=microprofile-graphql-quickstart \
-DclassName="org.acme.microprofile.graphql.FilmResource" \
-Dextensions="smallrye-graphql"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@phillip-kruger I am not sure of the name of the extensions. Is this correct?

Copy link
Member

Choose a reason for hiding this comment

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

Is it not quarkus-smallrye-graphql ?

Copy link
Member

Choose a reason for hiding this comment

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

----

The server will return the complete schema of the `graphQL` API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mentioned Graphiql in the documentation. Great feature 👍

@cemnura cemnura force-pushed the feature/graphql_docs branch from 1c996a2 to f792917 Compare May 11, 2020 20:38
@cemnura cemnura force-pushed the feature/graphql_docs branch from f792917 to 4c03df3 Compare May 21, 2020 22:15
Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

Awesome ! LGTM ! Thanks @cemnura !

@cemnura cemnura marked this pull request as ready for review May 26, 2020 14:21
cemnura and others added 2 commits May 26, 2020 17:06
And also include the generated configuration reference.
@gsmet gsmet force-pushed the feature/graphql_docs branch from 4c03df3 to 55043bb Compare May 26, 2020 15:17
@gsmet
Copy link
Member

gsmet commented May 26, 2020

I rebased and fixed a bunch of stuff in a separate commit. Will merge now.

@gsmet gsmet merged commit 031dfbd into quarkusio:master May 26, 2020
@gsmet
Copy link
Member

gsmet commented May 26, 2020

Thanks!

@gsmet gsmet added this to the 1.5.0.Final milestone May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants