-
-
Notifications
You must be signed in to change notification settings - Fork 227
Enhance split words #413
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
Enhance split words #413
Conversation
ba648d6
to
84192c6
Compare
Codecov Report
@@ Coverage Diff @@
## main #413 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 47 47
Lines 1549 1550 +1
=========================================
+ Hits 1549 1550 +1
Continue to review full report at Codecov.
|
321176c
to
d6cce56
Compare
I'm not totally convinced and I'm worried that this could get bikesheddy and keep becoming more specialized. Like, what if someone wanted HTTP200 to be http_200? Or in the golden records examples, type1 really is type_1 (meaning "the first type"). Happy to get input from others (e.g. @emann, @bowenwr) but I'm tempted to call it good enough and stop making changes to names unless something is really broken. Maybe allowing a manual override of specific values would be good though (e.g. s_3_config -> s3_config), I think we could add that through the normal config process with minor effort. |
I think we don't want to guess the words if there is no capital letter (i.e. if we're not seeing a PascalCase or camelCase string) because that means we would only split on numbers and will never be able to guess any kind of other word. Plus OpenAPI specifications authors will use some form of delimiters if there really is multiple words that need to be separated for readability anyway. As to the |
Oh I see, I misunderstood the change. For some reason I thought that it was making the snake case version of S3Config "s3_config" instead of "s_3_config" but I see now that it was already "s3_config" and the change here is to make the lowercase variant ("s3config") not be split at all. So my note about HTTP200 is irrelevant, both in main and in this branch it will be I agree with detecting the use of case and splitting based on that, sounds good. Let's just change |
d6cce56
to
999a81d
Compare
Ah, done, thanks for the hint! |
Latest on main breaks e2e 😬. I made a branch |
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.
Good to go as soon as the golden record conflict is resolved!
Keep "Response200" as "response_200", but don't make "s3config" become "s_3_config". :)
4bf52d5
to
8199585
Compare
Rebase went fine on my side, not sure what you meant. :) |
(and yeah, I did the |
Keep "Response200" as "response_200", but don't make "s3config" become "s_3_config". :)
Related to the comments here: #375 (comment)