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

Docs for classes in apollo-api module #444

Merged
merged 13 commits into from
Apr 27, 2017

Conversation

VisheshVadhera
Copy link
Contributor

Closes #435

@@ -254,6 +382,9 @@ public ScalarType scalarType() {
}
}

/**
* Abstraction for a Field representing a conditional type.
*/
public static final class ConditionalTypeField extends Field {
Copy link
Contributor Author

@VisheshVadhera VisheshVadhera Apr 21, 2017

Choose a reason for hiding this comment

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

I feel docs can certainly be improved for ConditionalType. Can you suggest better docs here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Conditional Type used for parsing inline fragments or fragments. Here is an example how it used:

final Field[] fields = {
        Field.forConditionalType("__typename", "__typename", new Field.ConditionalTypeReader<Fragments>() {
          @Override
          public Fragments read(String conditionalType, ResponseReader reader) throws IOException {
            return fragmentsFieldMapper.map(reader, conditionalType);
          }
        })
      };

It means that first field __typename will be read and then passed to another nested mapper along with reader that will decide by checking conditionalType what type of fragment it will parse.

@VisheshVadhera VisheshVadhera changed the title Feature apollo api docs Docs for classes in apollo-api module Apr 21, 2017
*
* Field can refer to: <b>GraphQL Scalar Types, Objects or List</b>. For a complete list of types that a Field
* object can refer to see {@link Field.Type} class.
*/
public class Field {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this class should be moved under internal package as it used internally by generated mappers

@@ -1,5 +1,8 @@
package com.apollographql.apollo.api;

/**
* Represents a custom graphQL scalar type
Copy link
Contributor

Choose a reason for hiding this comment

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

GraphQL

/** TODO **/
/*
* ResponseReader is responsible for converting a field object to another object of type T.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change this to: ResponseReader is an abstraction for reading GraphQL fields.

@@ -2,7 +2,9 @@

import java.io.IOException;

/** TODO */
/**
* * ResponseFieldMapper is responsible for mapping the responses returned by the server back to data.
Copy link
Contributor

Choose a reason for hiding this comment

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

one * is redundant.

ResponseFieldMapper is an abstraction for mapping the response data returned by the server back to generated models.

String queryDocument();

/** TODO */
/**
* Returns the Variable object associated with this GraphQL operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Returns the variables associated with this GraphQL operation.

public interface Operation<D extends Operation.Data, T, V extends Operation.Variables> {
/** TODO */
/**
* Returns the raw graphQL operation string.
Copy link
Contributor

Choose a reason for hiding this comment

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

GraphQL

V variables();

/**
* Returns a mapper that maps the server response back to an object of type D.
Copy link
Contributor

@sav007 sav007 Apr 21, 2017

Choose a reason for hiding this comment

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

Returns a mapper that maps the server response data to generated model data class {link D}

ResponseFieldMapper<D> responseFieldMapper();

/**
* Converts the object representing the server response to another type.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wraps generated response data class {link D} with another class, example Optional.fromNullable(data)

/** TODO */
/**
* Abstraction for the variables which are a part of the GraphQL operation. For example, for the following GraphQL
* operation, Variables represents an abstraction for GraphQL variables '$type' and '$limit' their values:
Copy link
Contributor

Choose a reason for hiding this comment

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

... variables represents values for GraphQL '$type' and '$limit' variables:

@VisheshVadhera
Copy link
Contributor Author

@sav007 Made the requested set of changes. If everything looks fine, we can go ahead and merge it.

public interface ResponseReader {
<T> T read(Field field) throws IOException;
<T> T read(com.apollographql.apollo.api.internal.Field field) throws IOException;
Copy link
Contributor

Choose a reason for hiding this comment

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

missing import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Totally missed that.

Copy link
Contributor

@sav007 sav007 left a comment

Choose a reason for hiding this comment

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

lgtm, small comment
@digitalbuddha @marwanad fresh look at this PR

public String message() {
return message;
}

/**
* Returns the location of the error in the query.
Copy link
Contributor

Choose a reason for hiding this comment

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

operation instead of query? (To cover mutation cases too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right.

@@ -5,28 +5,60 @@

import javax.annotation.Nonnull;

/** TODO */
/**
* Operation represents an abstraction for a GraphQL operation (mutation or query).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove Operation to keep it consistent with he doc for Mutation

@VisheshVadhera
Copy link
Contributor Author

@sav007 @marwanad Have made the requested set of changes. If it looks fine, we can merge it.

@@ -15,10 +17,16 @@ public Error(String message, @Nullable List<Location> locations) {
this.locations = locations;
}

/**
* Returns the error message returned by the server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Returns server error message.

public interface Operation<D extends Operation.Data, T, V extends Operation.Variables> {
/** TODO */
/**
* Returns the raw GraphQL operation string.
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick String

class Variables {
protected Variables() {
}

@Nonnull protected Map<String, Object> valueMap() {
@Nonnull public Map<String, Object> valueMap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why did this change to public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We moved the Field class to the internal package (which was earlier in the same package as the Operation interface) and Field class was using valueMap method. Hence it had to be changed to public.

@@ -1,5 +1,7 @@
package com.apollographql.apollo.api;

/** TODO */
/**
* Represents an abstraction for GraphQL query that will be sent to the server.
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer comments like Represents a custom GraphQL scalar type rather than things that mention something is an abstraction. So this one I think sounds better as "Represents a GraphQL query that will be sent to a server"

Copy link
Contributor

@digitalbuddha digitalbuddha left a comment

Choose a reason for hiding this comment

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

small nitpicks :-)

@VisheshVadhera
Copy link
Contributor Author

VisheshVadhera commented Apr 27, 2017

@sav007 Tests in the CodegenTest.kt class are failing due to moving the Field class to the internal package. Any idea how this can be fixed? Don't have much experience with Kotlin.

@sav007
Copy link
Contributor

sav007 commented Apr 27, 2017

@VisheshVadhera I am so sorry but with latest custom resolver feature, we no longer need to hide Field in internal package, could you please revert it back. Sorry.

@VisheshVadhera
Copy link
Contributor Author

@sav007 Reverted :)

@sav007
Copy link
Contributor

sav007 commented Apr 27, 2017

Merging

@sav007 sav007 merged commit 65c4fff into apollographql:master Apr 27, 2017
@VisheshVadhera VisheshVadhera deleted the feature-apollo-api-docs branch May 13, 2017 10:24
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.

4 participants