ebowman / api-first-hand

API-First bootstrapping tool for building RESTful web services from a Swagger/OpenAPI spec
MIT License
142 stars 22 forks source link

Odd/erroneous choices for types #58

Closed alexec closed 5 years ago

alexec commented 7 years ago

I'm seeing some odd types appearing in generated code.

type CalcAmount = Option[BigDecimal]

This makes sense for the field calcAmount. However, any time Option[BigDecimal] is needed, the same type is used.

expensesReceived: CalcAmount

IHMO it would be preferable to use Option[BigDecimal] as the type, than to choose these odd names.

gipeshka commented 7 years ago

I agree, having several optional boolean in request you end up with a misleading piece code:

type Some_feature_enabled = Option[Boolean]
...
some_feature_enabled: Some_feature_enabled,
some_other_feature_disabled: Some_feature_enabled
slavaschmidt commented 7 years ago

Makes sense, thanks for bringing it up!

SergeyNovikovDH commented 7 years ago

Would it make sense to leave Option[Boolean] (all options of primitives) as it is? I think type "Some_feature_enabled" is actually more confusing

alexec commented 7 years ago

I've updated the description to make it clearer what the issue is.

The types alias choice is odd. See @gipeshk for another example.

This should to be to make the code clearer. IHMO, this is actually a bug.

But I also think that having type aliases for simple types, such as Boolean or Option[Boolean], is complicated, and making that change would address the other problem at the same time.

gipeshka commented 7 years ago

I would consider Container to be not complex so that we would never replace arrays and opts

slavaschmidt commented 7 years ago

Well, we were thinking it will provide better readability, but then we had so many different types that we decided to "deduplicate" them... The problem with "simple" types that they can be nested, so what should be done in the case, for example of List[Option[_]]? Should it be type alias by itself or better a List of type aliases?

gipeshka commented 7 years ago

IMO containers should be present as they don't reduce readability until you have really crazy structures, which itself is a problem.

On the other hand if I had a way to influence the naming strategy it could help, e.g. provide name for my special Option[List[List[Option]]] type, wdyt?

SergeyNovikovDH commented 7 years ago

@slavaschmidt, List[Option[_]] LGTM

gipeshka commented 7 years ago

There could be a sbt parameter saying "please don't reduce containers to types"

slavaschmidt commented 7 years ago

@gipeshka yep, with the parameter that's exactly what I'm aiming to about providing custom type aliases... I'm not quite sure if it's worth the effort

gipeshka commented 7 years ago

another thing, I suppose api-first-hand should build an application with the same structure as defined in swagger files. First, it means that if user has really complex and not well structured swagger file this will force him to structure it better. Second, user will operate the same types as defined by definitions section in swagger file.