-
Notifications
You must be signed in to change notification settings - Fork 55
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
Add additional capitalization rules #463
base: main
Are you sure you want to change the base?
Conversation
👷 Deploy request for apollo-ios-docc pending review.Visit the deploys page to approve it
|
👷 Deploy request for eclectic-pie-88a2ba pending review.Visit the deploys page to approve it
|
675ba12
to
2ee1ff4
Compare
import Foundation | ||
|
||
public enum CapitalizationRule: Codable, Equatable { | ||
case uppercase(regex: String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if a regex pattern is the right way to handle this vs allowing explicit declaration of the type you want to change capitalization for and what you want it to be through a dictionary similar to how we handle the schema type renaming.
Thoughts @AnthonyMDev @calvincestari ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. That's a good question. But we aren't just talking about types here, we're also talking about their properties.
If you used .uppercase(regex: "id"), that could also result in situations where a property
isHiddengets changed to
isHIDden, for example. You would need to make your regex
^id$if you wanted to just match the exact string
id`, which I guess is alright, but not super easy to figure out if you're not very familiar with regex.
However, I think it's likely that there will be situations where you want to tell the codegen engine "uppercase all instances of the letters 'UUID'. This may show up in multiple different property names, and is less likely to have conflicts where it shows up in the middle of another word. A regex would be helpful in those situations.
There is also situations where you want to capitalize a term in a specific way, like GraphQL
. I think there should be a way to do that too. This doesn't fall into camelcase
exactly correctly.
I think a more robust API would allow users to use either a regex or a direct string literal and allow them to choose a common casing strategy or a direct replacement string.
struct CapitalizationRule {
enum Term {
case string(String)
case regex(String)
}
enum Strategy {
case uppercase
case lowercase
case titlecase
case camelcase
case pascalcase
case replace(String)
}
let term: Term
let strategy: Strategy
}
This would allow for all of the desired capabilities to exist. My only concern is that the replace
strategy allows for way more use cases outside of just capitalization. It might be used as a hacky workaround for other desired name changes that are not the intended usage of this API.
What do you think @tonyarnold @BobaFetters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quite like this proposed API! I'll update the PR to reflect it.
We could potentially leave .replace(…)
out of the strategies?
Edit: I've updated the API in 0cda6f0, but left out the replacement strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this up @tonyarnold!
We need to start by defining the scope of where this applies. This would definitely apply to:
- Field property accessor names
- Generated selection set type names
Should it also apply to schema types? It does seem reasonable that you would expect it to. But we also have the ability to rename schema types on an individual basis, so that already accounts for this functionality on schema types. It doesn't feel great to me to have multiple ways of doing the same thing. On the other hand, I could see it being confusing if you use this, expecting it to capitalize your schema types and it doesn't. I think I lean towards going ahead and applying this to schema types, but would appreciate feedback.
import Foundation | ||
|
||
public enum CapitalizationRule: Codable, Equatable { | ||
case uppercase(regex: String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. That's a good question. But we aren't just talking about types here, we're also talking about their properties.
If you used .uppercase(regex: "id"), that could also result in situations where a property
isHiddengets changed to
isHIDden, for example. You would need to make your regex
^id$if you wanted to just match the exact string
id`, which I guess is alright, but not super easy to figure out if you're not very familiar with regex.
However, I think it's likely that there will be situations where you want to tell the codegen engine "uppercase all instances of the letters 'UUID'. This may show up in multiple different property names, and is less likely to have conflicts where it shows up in the middle of another word. A regex would be helpful in those situations.
There is also situations where you want to capitalize a term in a specific way, like GraphQL
. I think there should be a way to do that too. This doesn't fall into camelcase
exactly correctly.
I think a more robust API would allow users to use either a regex or a direct string literal and allow them to choose a common casing strategy or a direct replacement string.
struct CapitalizationRule {
enum Term {
case string(String)
case regex(String)
}
enum Strategy {
case uppercase
case lowercase
case titlecase
case camelcase
case pascalcase
case replace(String)
}
let term: Term
let strategy: Strategy
}
This would allow for all of the desired capabilities to exist. My only concern is that the replace
strategy allows for way more use cases outside of just capitalization. It might be used as a hacky workaround for other desired name changes that are not the intended usage of this API.
What do you think @tonyarnold @BobaFetters?
2ee1ff4
to
0638c5f
Compare
0638c5f
to
0cda6f0
Compare
As discussed in apollographql/apollo-ios#3419, I'm proposing changes to add additional, configurable rules around capitalisation of type, property and other names via code-gen, such that I can control the output of
Id
to beID
, and similar.This is an initial pass to gather feedback on the configuration changes, and do some bike shedding prior to implementing the underlying feature.