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

Fix generation of nested structures with the same name results in compiler errors #191

Merged

Conversation

sav007
Copy link
Contributor

@sav007 sav007 commented Feb 13, 2017

Closes #169

@@ -61,9 +61,9 @@ public String queryDocument() {
}

public interface Data extends Operation.Data {
@Nullable Hero hero();
@Nullable HeroDetailQuery$ heroDetailQuery();
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if nested twice? tbh I'd prefer HeroDetailQuery$$1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will be HeroDetailQuery$$

Copy link
Contributor

Choose a reason for hiding this comment

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

And just for clarity 5 would be Query$$$$$ so just as example imagine you see following in logs...
NullPointerExxeption: Foo$$$$ is null

Will you be able to tell without dragging your finger in screen whether that was 4 or 5 dollar signs.
I don't feel strongly about it but can see it leading to confusing traces. As example rxjava does Action0 - Action9

Copy link
Contributor Author

@sav007 sav007 Feb 13, 2017

Choose a reason for hiding this comment

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

Not sure why we initially decided to go with $, but it's really easy to change in compiler. But the idea is that in most cases you won't have such issue with more than 2-3 nested classes with the same name. But I don't have any strong opinion for this as Hero4 and Hero$$$$ from my perspective is the same evil, and for sure I will try to avoid such queries with the same nested names (will use aliases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@digitalbuddha so are we changing $ to number or leaving as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer number but you make the call :-)

@digitalbuddha
Copy link
Contributor

Now you can blame me if people don't like it ☺️

@digitalbuddha
Copy link
Contributor

com.apollographql.android.compiler.CodeGenTest >  generateExpectedClasses[pojo_fragment_with_inline_fragment] FAILED
    org.junit.ComparisonFailure at CodegenTest.kt:31

@sav007 sav007 merged commit 18a84c5 into apollographql:master Feb 15, 2017
@sav007 sav007 deleted the bug-169/unique-type-name-operation branch February 15, 2017 16:02
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.

3 participants