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

Bring back all the pluralization #1879

Merged

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Jul 21, 2021

bring-back-all-the-things

This is a revert of 42a75f2 with some changes

  • InflectorKit is now sourced from the origin instead of the apollographql fork
  • InflectorKit is pinned to the next minor version of 1.0.0
  • PluralizerTests was updated to comply with deprecated function names
  • Update PluralizerTests function names to match Update tests to follow naming convention #1849

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 for doing this @calvincestari!

@@ -83,7 +84,7 @@ public struct ApolloCodegenOptions {
/// - codegenEngine: The code generation engine to use. Defaults to `CodeGenerationEngine.default`
/// - includes: Glob of files to search for GraphQL operations. This should be used to find queries *and* any client schema extensions. Defaults to `./**/*.graphql`, which will search for `.graphql` files throughout all subfolders of the folder where the script is run.
/// - mergeInFieldsFromFragmentSpreads: Set true to merge fragment fields onto its enclosing type. Defaults to true.
/// - modifier: [EXPERIMENTAL SWIFT CODEGEN ONLY] - The access modifier to use on everything created by this tool. Defaults to `.public`.
/// - modifier: The access modifier to use on everything created by this tool. Defaults to `.public`. Only used by the Swift code generation engine.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now. I think when we release the swift codegen, we will need to keep support for legacy codegen for a while, but that it should probably have its own set of options entirely so we can diverge more easily. But that's not a concern for today.

Sources/ApolloCodegenLib/ApolloCodegenOptions.swift Outdated Show resolved Hide resolved
@calvincestari calvincestari added this to the MM-2021-07 milestone Jul 21, 2021
@calvincestari calvincestari merged commit a7d11c8 into release/1.0-alpha-incubating Jul 21, 2021
@calvincestari calvincestari deleted the bring-back-all-the-pluralization branch July 21, 2021 19:07
@martinbonnin
Copy link
Contributor

Wait, isn't all this about singularization 🤔 ?

@calvincestari
Copy link
Member Author

InflectorKit provides both and Pluralizer wraps them.

@martinbonnin
Copy link
Contributor

But we don't really need to pluralize? Or do we?

@calvincestari
Copy link
Member Author

I am not sure - @designatednerd may have a better answer to that question.

@martinbonnin
Copy link
Contributor

martinbonnin commented Jul 21, 2021

I think we use it for things like

{
  hero {
    # friends is a list type
    friends {
      name
    }
  }
}

to generate a Friend struct/class (and not Friends):

struct Friend {
  let name: String
}

I don't think we need the other way

@designatednerd
Copy link
Contributor

We primarily use it for singularization. Somewhere in the back of my mind I remember there was some point where we were also pluralizing but i'm having a memory read error on where.

@designatednerd
Copy link
Contributor

Aha, found it: #955, was trying to pluralize ETA as Etum

@calvincestari
Copy link
Member Author

@designatednerd - with this merged we can probably drop our fork of InflectorKit.

@designatednerd
Copy link
Contributor

Yes, though I would not recommend deleting it since then it won't resolve for older versions of the lib

@hwillson hwillson removed this from the MM-2021-07 milestone Jul 29, 2021
calvincestari added a commit that referenced this pull request Aug 13, 2021
* Revert "Remove inflection option, pluralizer and dependency"
* Move from InflectorKit fork to origin with 1.0.0 minimum
* Update to comply with InflectorKit 1.0.0 deprecations
* Enable code generation options to accept additional inflection rules
* Update PluralizerTest function names to match #1849
* Shuffle parameter documentation order to match parameter input order
calvincestari added a commit that referenced this pull request Aug 23, 2021
* Revert "Remove inflection option, pluralizer and dependency"
* Move from InflectorKit fork to origin with 1.0.0 minimum
* Update to comply with InflectorKit 1.0.0 deprecations
* Enable code generation options to accept additional inflection rules
* Update PluralizerTest function names to match #1849
* Shuffle parameter documentation order to match parameter input order
calvincestari added a commit that referenced this pull request Sep 3, 2021
* Revert "Remove inflection option, pluralizer and dependency"
* Move from InflectorKit fork to origin with 1.0.0 minimum
* Update to comply with InflectorKit 1.0.0 deprecations
* Enable code generation options to accept additional inflection rules
* Update PluralizerTest function names to match #1849
* Shuffle parameter documentation order to match parameter input order
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.

5 participants