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

[Client] Allow query to specify every subtype of an interface #1103

Conversation

AlixBa
Copy link
Contributor

@AlixBa AlixBa commented Oct 19, 2021

Rationale: We would like compilation to fail if there a new enum entry and we don't implement a selection builder

@AlixBa AlixBa force-pushed the add-option-to-avoid-default-impl-on-interface-types branch 2 times, most recently from 0d4f08c to eedf601 Compare October 19, 2021 14:56
@ghostdogpr
Copy link
Owner

ghostdogpr commented Oct 20, 2021

So, I've recently implemented this PR for unions. Basically it generates 2 methods instead of 1, one with all union subtypes optional and the other where they are mandatory. How about doing the same for interfaces? It would be consistent and would prevent an additional configuration parameter. Also you would be free to use a mix of them in your code, which is more flexible than a single configuration. What do you think?

@AlixBa
Copy link
Contributor Author

AlixBa commented Oct 21, 2021

I'll give it a shot :)
I'd rather push something unified at a caliban writer level than another CLI option!

EDIT: I see that before, on union subtypes, it was mandatory and you added a xxxOption method with default. For interfaces it will be the other way around. And as I guess we don't want to break compatibility how should I name this new method that will enforce all parameters to be set?

@ghostdogpr
Copy link
Owner

Hmm I'd rather break and be consistent. We can mention it clearly in the release notes and it won't be hard to fix. It won't break code immediately for users unless they generate the code during compilation. Sounds reasonable?

@ghostdogpr
Copy link
Owner

If we break it, I'll give #237 a thought and see if I find a good solution. It might also break so better to do all at once if possible.

@AlixBa
Copy link
Contributor Author

AlixBa commented Oct 21, 2021

I'll have something by the end of the day 🤞 (in France)

@ghostdogpr
Copy link
Owner

ghostdogpr commented Oct 21, 2021

@AlixBa yeah, that's right.

And it would be nice to add onInterfaceName: Option[SelectionBuilder[InterfaceName, A]], to select common fields from the interface (and generate that code, it doesn't exist currently). I guess it makes sense only in methodOption, not on the other one?

@AlixBa AlixBa changed the title Option to disable default implementation on interface types selection builder [Client] Allow query to specify every subtype of an interface Oct 21, 2021
@AlixBa AlixBa force-pushed the add-option-to-avoid-default-impl-on-interface-types branch from eedf601 to ecc1185 Compare October 21, 2021 14:04
@AlixBa
Copy link
Contributor Author

AlixBa commented Oct 21, 2021

I've updated with the first goal of this MR.
I'll try to impl #237 :)

@AlixBa AlixBa force-pushed the add-option-to-avoid-default-impl-on-interface-types branch from ecc1185 to 0678ae1 Compare October 22, 2021 18:53
@AlixBa AlixBa force-pushed the add-option-to-avoid-default-impl-on-interface-types branch 2 times, most recently from 899b9e7 to 450dfe3 Compare November 10, 2021 16:59
@ghostdogpr
Copy link
Owner

The test error comes from this code generated in writeView:

case (field @ FieldTypeInfo(_, _, _, interfaceTypes, _, _, Some(_)), fieldName) if interfaceTypes.nonEmpty =>
          val tpe = genericSelectionFieldTypesMap(field)
          interfaceTypes.map(intType => s"${fieldName}On$intType: Option[SelectionBuilder[$intType, $tpe]] = None")

I guess it doesn't need to be an Option anymore (like for unions just a few lines above)?

You can run scripted in sbt to test it.

@AlixBa AlixBa force-pushed the add-option-to-avoid-default-impl-on-interface-types branch from 450dfe3 to 09905c0 Compare November 15, 2021 16:55
@AlixBa AlixBa force-pushed the add-option-to-avoid-default-impl-on-interface-types branch from 09905c0 to e84dfac Compare November 18, 2021 11:07
@ghostdogpr
Copy link
Owner

Btw is this PR ready to review? I wasn't sure 😅

@AlixBa
Copy link
Contributor Author

AlixBa commented Nov 24, 2021

Ups, sorry @ghostdogpr
It is, indeed

Copy link
Owner

@ghostdogpr ghostdogpr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ghostdogpr ghostdogpr merged commit 8727fce into ghostdogpr:master Nov 25, 2021
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.

2 participants