sbcgua / ajson

Yet another json parser serializer for ABAP
MIT License
50 stars 15 forks source link

New mapper approach #117

Closed sbcgua closed 1 year ago

sbcgua commented 2 years ago

@jrodriguez-rc could you have a look ? This is related to my comments in #112. In a nutshell:

you initiated the mapper idea and, I assume, using it the most :) so I'd like to know your opinion.

No need to review all the code, and it is not finished anyway. Have a look at readme change, then zif_ajson_mapping, then maybe unit tests for what was not clear from the doc.

jrodriguez-rc commented 2 years ago

This works only from a JSON source and does a JSON node renaming, isn't it? I see the changes well but if you allow me some suggestions:

sbcgua commented 2 years ago

This works only from a JSON source and does a JSON node renaming, isn't it?

Yes, I came to a conclusion that it is the only operation needed in fact. E.g. parse external json with camelcase -> create_from( to_snake ) -> apply in abap Or create "abap" json -> create_from( to_camel ) -> send to ext. API Both scenarios well covered with one shot

rename_node ... Yes, I didn't like rename_field too. I though about rename_attribute or rename_attr. But rename_node seems to be a good and clear candidate, and go well with other used terms in code. I'll consider. Thanks !

zcl_ajson_utilities=>merge Hmm, and what's there ?

BTW, one question. The existing upper/lower case mappers also accept the direct name mapping structure. So they can do both conversions. Which is nice but not very clean from the architecture perspective. Alternative is running create_from twice. Which is OK imho, but a bit bulky. Or, maybe implement queue of mappers (and ... also filters just by the way). What do you think ? So the code in 7.40 syntax would look something like

json_new = zcl_ajson=>create_from(
  ii_json = json_old
  it_processors = value #(
    ( zcl_ajson_mapping=>to_snake( ) )
    ( zcl_ajson_mapping=>rename( lt_my_rename_pairs ) )
    ( new lcl_my_json_filter( ) )
  ) ).

... for sure keeping the existing params at least for a while or maybe permanently. The queue, if supplied, would just be preferred

Does it make sense or smells overengineering ? Again, I'm not using mappers a lot personally, it just my feeling that it would be better.

jrodriguez-rc commented 2 years ago

zcl_ajson_utilities=>merge this method seems to map two JSON. Maybe one is in "lowercase" format and the other in "uppercase" and I want that the resulting JSON has a "lowercase" format.

I wouldn't say I like the implementation of a queue or something like that, anyone can implement the mapping they want

CLASS zcl_json_custom_mapping IMPLEMENTATION.

  METHOD zif_ajson_mapping~rename_field.

    DATA(li_upper) = zcl_ajson_mapping=>create_upper_case( ).

    li_upper->rename_field( EXPORTING is_node = is_node
                            CHANGING  cv_name = cv_name ).

    DATA(li_rename) = zcl_ajson_mapping=>create_rename( VALUE #( ( from = 'FIELDNAME1' to = 'FIELDNAME2' ) ) ).

    li_rename->rename_field( EXPORTING is_node = is_node
                             CHANGING  cv_name = cv_name ).

  ENDMETHOD.
sbcgua commented 2 years ago

... reimplemented to_camel in the new approach and stumbled upon this line https://github.com/sbcgua/ajson/blob/master/src/zcl_ajson_mapping.clas.locals_imp.abap#L158

@jrodriguez-rc could you please comment what was the idea ?

sbcgua commented 2 years ago

I guess it's ready. Let it calm a bit. But generally should be +- final. See the docs for the summary.

sbcgua commented 2 years ago

@larshp BTW there seems to be a minor abaplint bug. It claims that Statement does not exist in ABAPv702(or a parser error), "cl_abap_unit_assert" (parser_error) [E]. This happens only for the line with value #() which is not 702 of course, yet the cl_abap_unit_assert itself is a false positive. Also there is another message about chaining below. Cannot check on 702 but ... it is not a new syntax afaik