to_abap: Mapping fields

jrodriguez-rc commented 3 years ago

It seems that there is an error in abaplint calculating the indentation in local classes inside TRY.

I open an issue in abaplint https://github.com/abaplint/abaplint/issues/1652

jrodriguez-rc commented 3 years ago

This setting is correct https://github.com/sbcgua/ajson/blob/master/abaplint.json#L121?

sbcgua commented 3 years ago

Ah, nice feature :) :+1:

One thing to consider, there is #11 - auto camel/decamel case - which rings very close to mapping. Maybe makes sense to think about how they will co-exist. E.g. camel/decamel will autocreate a mapping. And if so, will it impact the interface. (separate feature for sure, just to mention here)

Also. The type defined in the interface. There is #39. I'll create a common interface. Maybe makes sense to unify all types there. So the type placement may change soon.

The try-catch indentation. Yes, this is intentional. I believe this is the right way to indent try-catch. Pretty printer is wrong.

P.S. I'd suggest separating style changes from features. It may interfere with other changes and is more difficult to review. It is OK to leave it as is for the moment, but might need some conflict resolution/rebasing.

sbcgua commented 3 years ago

Also. The type defined in the interface. There is #39. I'll create a common interface. Maybe makes sense to unify all types there. So the type placement may change soon

Maybe ... field mapping is not a parameter of to_abap. But instead an assignable attribute of the class ? Is it possible that mapping changes during the lifetime of an instance ? Just thinking

jrodriguez-rc commented 3 years ago

About moving all types (and constants) to an independent interface is a good idea, I defined mapping type in zif_ajson_reader because zcl_ajson is private, but the idea is to use it in writer too. It could be zif_ajson

For this and camel case (can add more options, like always upper/lower case) I think to leave it in the corresponding methods, to support all possible cases.

sbcgua commented 3 years ago

What if ... mapping is a class. And is assigned with a separate method. (these are 2 independed features in fact)

  map = zcl_ajson_map=>new( )
  map->add( 
    json = 'json_field'
    sap = 'sap_field' ).
  " OR
  map->add( 'json_field=sap_field' ).
  " OR EVEN
  map->add( 'json_field=sap_field, json_field2=sap_field2' ).
  " AND ALSO
  map = zcl_ajson_map=>auto_camel( cl_abap_typedescr=>describe_by_xxx( my_struc ) )

  " AND THEN
  lo_ajson->assign_field_map( map )->to_abap( importing ev_container = ls_result ) " To avoid import/export syntax -> though arguable ...

  " OR MAYBE (though overengineered)
  lo_ajson->assign_field_mapPER( map )->to_abap ...
  " and the code inside is
  if mi_mapper is bound. " Interface, not a class
    ls_seg = mi_mapper->map_to_abap( ls_seg ). " though needs a performance test just in case ...
  endif.

sbcgua commented 3 years ago

would you mind if I merge #42 before this one ? (May require some rebase because of styling changes ...)

jrodriguez-rc commented 3 years ago

It's ok, I will rewrite this using objects.

We can use an interface zif_ajson_field_mapping with an unique method convert_field

This allows to create custom mappings.

I'll create some proposal tomorrow, meanwhile this PR can be closed

sbcgua commented 3 years ago

Yeap, custom mapping is what I had in mind. Hopefully it is not over engineered. Feel free to suggest other opinions, the above is a discussion. I just want to make it universal and convenient.

One thing: convert_field - I'd think about another name. A) the object will already be called field_mapper or similar (so "field" is unnecessary) B) The mapper should support 2 way convertion ! json->sap, sap->json ... so ... to_sap and to_json ? or sap2json/json2sap ? No strong opinion here

sbcgua / ajson

to_abap: Mapping fields #41