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

Support wider range of chars in param names in codegen #3059

Conversation

hughsimpson
Copy link
Contributor

@hughsimpson hughsimpson commented Jul 21, 2023

It's common to have openapi specs where a parameter or path contains characters that aren't legal in scala val names (e.g. path = member/{user-uuid} or operationId = get-member). This is a minor change that supports a much wider range of openAPI input without changing generated endpoint names too much - all non-legal identifiers are stripped of non-legal chars and converted to camelCase with illegal chars constituting a 'break'. Any legal identifier (as specificed by e.g. operationId) will be retained as-is - that is, we only rewrite the endpoint names where they would otherwise be illegal.

@hughsimpson hughsimpson changed the title Support wider range of chars in param names Support wider range of chars in param names in codegen Jul 21, 2023
@adamw
Copy link
Member

adamw commented Jul 23, 2023

I guess this is exclusive with #3060?

@hughsimpson
Copy link
Contributor Author

hughsimpson commented Jul 23, 2023

It would certainly produce merge conflicts (although I don't mind resolving them at all). I can see how both might be useful together though (e.g. if operationId=some-kebab-case-name, or simply wasn't present on some operation object). If it came down to a straight choice between the two prs, however, I prefer #3060

@adamw
Copy link
Member

adamw commented Jul 23, 2023

So I'm wondering about this one, even if an operation's name/id has dashes or other symbols inside, maybe we should instead convert to the "Scala convention" that is camel case? I've never been a fan of the back-ticked names :)

@hughsimpson
Copy link
Contributor Author

Well that would certainly make for more consistent-looking variable names 😂. Yes, I think that makes sense if the regex match fails. I'll make that change and fix conflicts, but will probably be a bit later on, now.

@hughsimpson hughsimpson force-pushed the support_wider_range_of_chars_in_param_names branch from bd73f4e to b64c3d1 Compare July 23, 2023 21:32
@hughsimpson
Copy link
Contributor Author

hughsimpson commented Jul 23, 2023

Fiddled with it a bit - think it does what we want. AFK now but will push up a reformat or something to kick the tests if you don't get around to rerunning the hanged job before tomorrow morning or so.

Although TBH could be more aggressive here - not sure the first part is really relevant after this...

@adamw
Copy link
Member

adamw commented Jul 24, 2023

This looks good :) Restarted the build

@hughsimpson hughsimpson force-pushed the support_wider_range_of_chars_in_param_names branch 2 times, most recently from 1f4a0aa to 97a8d3c Compare July 24, 2023 07:19
@adamw adamw merged commit b155c87 into softwaremill:master Jul 24, 2023
@adamw
Copy link
Member

adamw commented Jul 24, 2023

Thanks again :)

@hughsimpson hughsimpson deleted the support_wider_range_of_chars_in_param_names branch July 24, 2023 10:41
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.

3 participants