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

feature: final class definitions for graphql codegen #3189

Merged
merged 1 commit into from
Sep 5, 2023

Conversation

Mordil
Copy link
Contributor

@Mordil Mordil commented Aug 12, 2023

Re-adds generated GraphQL operations to be marked as final through a new OutputOptions property: markClassesAsFinal.

This is to support fixing this in a backwards-compatible way, and still allowing people control over how they use the generated operation models.

This resolves #3183

@netlify
Copy link

netlify bot commented Aug 12, 2023

👷 Deploy request for apollo-ios-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 01010ac

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this! Implementation looks spot on to me! Just a couple things to consider.

  • We need to add the new option to the documentation of the CLI

  • I think we should also add the final keyword to local cache mutations if this option is true. That would be done in the LocalCacheMutationDefinitionTemplate.

  • @BobaFetters what do you think of the naming of this configuration option? I'm not coming up with a better name off the top of my head right now, but I feel like it's not clear what finalOperationDefinitions means without having to read the docs.

@Mordil
Copy link
Contributor Author

Mordil commented Aug 16, 2023

I just came up with markClassesAsFinal, but now I'm wondering if that name now pushes is so we should mark Mock definitions as final as well?

It'd probably be easier to make it so that it applies to all classes to start with, and if people say they want more granular control, later the options are split into further sub-options

@Mordil Mordil requested a review from AnthonyMDev August 16, 2023 01:05
@Mordil Mordil changed the title feature: final graphql operation definitions feature: final class definitions for graphql codegen Aug 16, 2023
@BobaFetters
Copy link
Member

Overall I think this looks great, thanks for taking this on!

As for naming, since the final keyword is a modifier thoughts on naming the option something like applyFinalModifier?

@Mordil
Copy link
Contributor Author

Mordil commented Aug 17, 2023

Overall I think this looks great, thanks for taking this on!

As for naming, since the final keyword is a modifier thoughts on naming the option something like applyFinalModifier?

I don't have a strong preference. It's what y'all think is best.

@BobaFetters
Copy link
Member

Overall I think this looks great, thanks for taking this on!
As for naming, since the final keyword is a modifier thoughts on naming the option something like applyFinalModifier?

I don't have a strong preference. It's what y'all think is best.

@AnthonyMDev thoughts?

@calvincestari
Copy link
Member

I just came up with markClassesAsFinal, but now I'm wondering if that name now pushes is so we should mark Mock definitions as final as well?

I'm less inclined to want final mock classes. I think the fact that they're intended to be test classes should allow for more complicated, do-as-you-want code.

It'd probably be easier to make it so that it applies to all classes to start with, and if people say they want more granular control, later the options are split into further sub-options

Yes I agree.

I'm happy with the config option named markClassesAsFinal, I think it's descriptive enough.

@AnthonyMDev
Copy link
Contributor

I felt like finalOperationDefinitions wasn't completely clear about what final meant in the context. But now markClassesAsFinal makes me confused about what Classes means. I don't think people using the codegen really know exactly what it is we are generating as classes vs structs.

What about markOperationDefinitionsAsFinal? I think that covers all our bases.

And I agree with @calvincestari that mocks should not be final.

@Mordil
Copy link
Contributor Author

Mordil commented Sep 1, 2023

I felt like finalOperationDefinitions wasn't completely clear about what final meant in the context. But now markClassesAsFinal makes me confused about what Classes means. I don't think people using the codegen really know exactly what it is we are generating as classes vs structs.

What about markOperationDefinitionsAsFinal? I think that covers all our bases.

And I agree with @calvincestari that mocks should not be final.

Again, I have zero preferences.

I'll update the PR with this new proposed name, at the same time that I'll squash the commits and update with the latest version of the dev branch.

Appreciate the feedback!

@AnthonyMDev
Copy link
Contributor

Thanks so much for this @Mordil! This looks great! Merging this in now, we'll get it out in the next release!

@AnthonyMDev AnthonyMDev merged commit 5f93599 into apollographql:main Sep 5, 2023
@Mordil Mordil deleted the final-operations branch October 5, 2023 18:39
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.

Why were GraphQLOperations changed from final to non-final?
4 participants