Allow overriding the output module name

andrenth / ocaml-swagger

Swagger 2.0 code generator for OCaml

37 stars 10 forks source link

Allow overriding the output module name #19

Closed carlosdagos closed 5 years ago

carlosdagos commented 5 years ago

Allow an optional parameter to override the default output module name which is based on the swagger document title. The value is provided via a parameter module_name via a string and it is still subject to snake_case.

The background is: I'm using this swagger spec which contains a title with spaces. The snake_case function is not ready for that. I first tried to rewrite that but it's quite important for the resulting output so I decided to leave it as it is. This is a bit of a safer change. Unfortunately it changes how the top-level call is made (updated on the README).

Happy to make changes as requested :+1:

carlosdagos commented 5 years ago

Bumped the version out of habit. Feel free to cherry pick.

rizo commented 5 years ago

@carlosdagos thanks for the PR and sorry for the delay.

I like the idea but have a couple of thoughts:

I'm not sure if module_name is the most informative name for this argument. It seems very generic. I'd rename it to something that maps more closely to the info.title key or potentially name it root_module_name.
The fact that it is still subject to snake_caseing might go against user expectations. If it's not too hard to change this I'd recommend doing it. Otherwise mentioning this in documentation would be needed at least.

Given that this changes the top-level call we'd need to update Kubecaml as well, but I can do it once this PR is merged.

rizo commented 5 years ago

Just for completeness I must mention an alternative approach that would also work. If we exposed the Swagger_t module in the public interface it would make it very easy to change the original title without the need for your PR.

This would require creating the swagger model and passing it to the codegen function, of course.

@carlosdagos what do you think?

carlosdagos commented 5 years ago

Hey @rizo,

Sorry for afk.

I think that exposing the Swagger_t sounds like a cool option. It makes sense to separate those concerns. I'll open a separate PR for that as soon as I get my hands on my laptop :smile:

Cheers

carlosdagos commented 5 years ago

Closing this one in favour of #20 :smile: