tkrajina / typescriptify-golang-structs

A Golang struct to TypeScript class/interface converter
Apache License 2.0
505 stars 87 forks source link

Add`.ManageType` #34

Closed shackra closed 3 years ago

shackra commented 3 years ago

This PR extends add .ManageType allowing developers add their own ts_type and ts_transformation rules for certain types, useful when you cannot add the tags to fields of third-party structs, like this one.

An existing unit-test, TestDate, was modified allowing test of this capability.

Fix #33

tkrajina commented 3 years ago

First of all, I agree about #33 . But at the moment I'm not yet 100% convinced about this implementation -- added a couple of comments. And I also see you're still experimenting :) Give me a couple of days to think about this before deciding if to merge it.

shackra commented 3 years ago

@tkrajina experimenting indeed, but this route I'm taking looks complicated more and more.

shackra commented 3 years ago

@tkrajina I'm trying to use another approach without changing .Add, could you comment my branch? I'm not really sure where the incoming types should be replaced with the information registered by the developer

shackra commented 3 years ago

I mean, I'm not that familiar with the internals, so I don't really know where should make use of the "managed" types and where the code should remain the same.

tkrajina commented 3 years ago

Here's my idea: https://github.com/tkrajina/typescriptify-golang-structs/tree/custom-types-transformations

When adding a new struct you can:

.Add(StructType{Type: ..., FieldOptions: map[reflect.Type]FieldOption{ time.Time: FieldOption{...} }

...and then all you need is to update the getFieldOptions method to either use the tag defined options (the method currently does only that) or find a custom FieldOption.

If we want to add global FieldOptions (for all models, not just one) -- this will also be handled in one place only -- in getFieldOptions.

What do you think?

PS. regarding your latest implementation -- yes it's much cleaner now.

shackra commented 3 years ago

PS. regarding your latest implementation -- yes it's much cleaner now.

:D

What do you think?

I was reading the code on your branch, .Add seems to remain the same for the user's POV, which is great. If we only have, at most, one point in the code which the usage of those registered/managed types takes place I believe it comes as a good approach, I don't any concerns with your proposal, but I wonder about this:

type FieldOptions struct {
    TSType      string
    TSTransform string
}

type StructType struct {
    Type         reflect.Type
    FieldOptions map[reflect.Type]FieldOptions
}

Why is FieldOptions a map which the key happens to be of the same type as Type? Maybe it is because is 2 am here and my eyes are burning :laughing: but I don't really get it. If we are searching for a particular StructType, there is no way around it, checking Type is a must, thus getting the FieldOptions using a reflect.Type as key is, well, of help? Maybe I'm missing some functionality you envision from this design :thinking:

tkrajina commented 3 years ago

Why is FieldOptions a map which the key happens to be of the same type as Type

Keys are the type of the field. StructType.Type is the type of the struct itself. I also updated my branch. It now has a global field type overload and a per-struct field-type overload.

shackra commented 3 years ago

Why is FieldOptions a map which the key happens to be of the same type as Type

Keys are the type of the field. StructType.Type is the type of the struct itself. I also updated my branch. It now has a global field type overload and a per-struct field-type overload.

Yeah, Early I was reading your comment and trying to make sens of your explanation, several hours later it hit me: Type field refers to (for instance) a third-party struct and the map refers to its fields.

In your code, what happens if a user makes several calls to .WithFieldTypeOpts with the same type for Type but different fields? Asking because maybe we want to override old registration with new data, or issue an error if a type was already registered

EDIT: It seems that no check is done before registering a type in .WithFieldTypeOpts.

tkrajina commented 3 years ago

Merged it but it's a mix of your PR and my branch. It has a ManageType function, but the second arg is a struct. And it has no automatic Date conversion -- I'll let users decide when/if to use that. Just changing it would suddenly change models for existing users.