Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: added object_type_name_prefix #1466
base: master
Are you sure you want to change the base?
feat: added object_type_name_prefix #1466
Changes from 3 commits
2dee4f2
0633b62
8cb9791
8423706
97dd7e5
b0e179f
673a282
10c53d3
0ea4636
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 completely sure but I think that even if the name comes from
field.name
we want to add the prefix... would be good to add a unit test for this.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.
Looking again at the code I see that we bypass
auto_camelcase
when the field name is explicitly defined, shouldn't we expect the same fortype_name_prefix
?I'm not sure neither to be honest
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.
yeah I saw that we bypass the auto camel case but I am not sure it makes sense to remove the prefixing... 🤷♂️
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.
Should this really be part of
type_name_prefix
, or should we rather add a meta optionfield_name_prefix
toObjectType
? Covers more use cases than this.The drawback is having to configure the same prefix in different places. But we are more exact as the
type_name_prefix
only applies to type names, not to fields.If we go for
field_name_prefix
, I agree with @tcleonard to add the prefix to all field names.If we go for
type_name_prefix
es I am not sure either.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.
This is to go from
camelCase
toPascalCase
as per convention for type names.Ideally we'd need a
auto_pascalcase
argument forSchema
to control this like we do withauto_camelcase
.It feels a bit overkill though, any thoughts on this?
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.
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 did that at first actually, but:
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.
mmmm that's a fair point! (that also means that if we have auto camel case on and we have
my_fieldIsGreat = Field(...)
it would becomemyFieldisgreat
because it does use.capitalize()
... should we change that @erikwrede in your opinion?)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.
@tcleonard great catch! I'm not sure about changing that since
my_fieldIsGreat
is invalid snake case and the contract forauto_camelcase
is to transform validsnake_case
toPascalCase
. So the specified behavior is currently 'undefined'. Would definitely be a breaking change, so we should ask for more opinions on Slack.For prefixes, we should expect the use of proper
PascalCase
.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 don't think there is a need to deal with the
auto_camelcase
case here as it is already dealt with in theget_name
.also note that in your current implementation where you don't
lower
the first character in the case we are not in auto camel case means the output is not exactly what you give in the doc:indeed that would be
MyPrefixInner
...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.
By default
auto_camelcase=True
so the query name would bemyPrefixInner
indeed isn't it?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.
yes but my point was if
auto_camelcase
is:True
->myPrefixInner
False
->MyPrefixInner
which doesn't seem very intuitive... no?
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 confused, what would be more intuitive in your view?
Because
MyPrefixInner
(PascalCase
) ->auto_camelcase=True
->myPrefixInner
(camelCase
)MyPrefixInner
(PascalCase
) ->auto_camelcase=False
->MyPrefixInner
(PascalCase
)Feels the most natural to me 🤔
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.
Gentle ping on that one @tcleonard