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

Generate Optional instead of null #322

Merged
merged 7 commits into from
Mar 16, 2017

Conversation

sav007
Copy link
Contributor

@sav007 sav007 commented Mar 15, 2017

Part of #290

Variables generation remains the same due to the plan of refactoring to simplify builder. So will be done in next PR.

Input object generation remains the same as they being used in serialization of request body via moshi.

@sav007 sav007 changed the title [WIP] Generate Optional instead of null Generate Optional instead of null Mar 15, 2017
@@ -4,6 +4,7 @@ import com.apollographql.android.api.graphql.Mutation
import com.apollographql.android.api.graphql.Operation
import com.apollographql.android.api.graphql.Query
import com.apollographql.android.api.graphql.ResponseReader
import com.apollographql.android.api.graphql.internal.Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

outside the scope of the PR, but I feel Optional shouldn't be in the internal package since we're exposing that API to consumers.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe external then?

Copy link
Contributor

Choose a reason for hiding this comment

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

that or maybe util or extension since we're making the use of Optional optional?
No preference, whatever works.

@sav007
Copy link
Contributor Author

sav007 commented Mar 15, 2017

@marwanad could you pls review again I add new extension to our plugin to switch on/off Optional generation

@@ -37,7 +40,11 @@ void generateClasses(IncrementalTaskInputs inputs) {
inputs.outOfDate(new Action<InputFileDetails>() {
@Override
public void execute(InputFileDetails inputFileDetails) {
new GraphQLCompiler().write(inputFileDetails.getFile(), outputDir, customTypeMapping, hasGuavaDep);
NullableValueType nullableValueType = NullableValueType.NONE;
Copy link
Contributor

@marwanad marwanad Mar 15, 2017

Choose a reason for hiding this comment

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

I think it would be better if we keep this logic part of the compiler. I feel making this class as dummy as possible and just passing all the information to the compiler would make it easier and faster to test for one (no need for a separate integration test) alongside keeping the generation logic in one place.

Might as well create a data class GraphQLCompilerArgs to wrap all the arguments and do these checks in GraphQLCompiler.write()?

Copy link
Contributor

@BenSchwab BenSchwab left a comment

Choose a reason for hiding this comment

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

This is great - thanks for making optional use confirugable

@sav007 sav007 merged commit 372a116 into apollographql:master Mar 16, 2017
@sav007 sav007 deleted the feature-290/generate-optional branch March 16, 2017 04:36
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.

5 participants