ABAP-Logger / ABAP-Logger

ABAP Logging as painless as any other language
MIT License
346 stars 111 forks source link

ABAP Logger v2 #157

Open mbtools opened 11 months ago

mbtools commented 11 months ago

Collecting breaking changes for an eventual v2.0.0

  1. Remove new and open methods from zcl_logger (replaced be zcl_logger_factory)
  2. Remove aliases from zcl_logger (not necessary if you use factory which returns an interface instance)
  3. Rename desc parameter to extnumber in zcl_logger_factory=>create_log and open_log
  4. Remove fullscreen and popup from zif_logger (replaced by display_fullscreen and display_as_popup)
  5. Move attributes from zif_logger to zcl_logger and add getter methods (#29)
  6. Rename zif_loggable_object to zif_logger_log_object to harmonize interface prefixes
  7. Rename importing parameters by removing i_ prefix
  8. Rename all returning parameters to result
  9. Stricter abaplint rules (no_prefixes, no_aliases, indentation, avoid_use, ...)
  10. Consistent error handling with exception class (and handle all exceptions of FM calls)
  11. Replace macros in test code
  12. Move display methods to zif_logger_ui and zcl_logger_ui

These changes should ensure a long future of ABAP Logger 😄. Feel free to comment or add suggestions.

jelliottp commented 11 months ago
  1. Rename desc parameter to description in zcl_logger_factory=>create_log and open_log

Regarding this one, I agree with renaming it but why not something like external_id? Since ultimately that's what it maps to.

Also, if you're doing breaking changes, I wonder if there is a way to resolve what I brought up in https://github.com/ABAP-Logger/ABAP-Logger/issues/123? Maybe this isn't a big issue for others, but having an instance method for create_log( ) or similar would help simplify a lot of implementations.

mbtools commented 10 months ago

Yes, I updated it to extnumber (i.e. balhdr-extnumber)

I created #159 for the injectors. We can do that without breaking things.

christianguenter2 commented 10 months ago

I'd add

AlexandreHT commented 10 months ago

I'm not sure what you're referring to for "consistent error handling with exception class"?

I'd add: move the log entry manipulation to a separate functionnal class (cf. https://github.com/ABAP-Logger/ABAP-Logger/issues/67#issuecomment-738303553) (zcl_abap_logger_functions?) Which would enable to separate the part that formats data and the part that logs it.

mbtools commented 10 months ago

error handling:

For example, if you look at zif_logger, none of the methods raise exceptions. All methods that could possibly have an error, should be defined with raising zcx_logger.

There are several function calls that do not check all exceptions. after call function it should have if sy-subrc <> 0. zcx_logger=>raise(...). endif.

For that to happen, zcx_logger should be enhanced like zcx_abapgit_exception with various raise... methods which makes the exception very easy to use throughout the project.

AlexandreHT commented 9 months ago

Ok. We'll just have to handle the case where the program tries to log an zcx_logger exception. Considering the fact that this repo is much smaller than abapgit, it seems to me that a simple unique exception would be enough but you're in a better position as a contributor to the abapgit repo to weight the pro/cons.

mbtools commented 9 months ago

good point of logging zcx_logger. I don't want to add exceptions, just raise the one we have.

nununo commented 9 months ago

Ideally zif_logger shouldn't have anything to do with UI. So... I don't think methods like display_fullscreen should have a place here. Instead you could have, for example, zif_logger_ui implemented by zcl_logger_ui_fullscreen which receives zif_logger in its constructor.

nununo commented 9 months ago

We are considering introducing ABAP-Logger to our project but if you're thinking about creating a V2 which will have breaking changes we should probably wait...

mbtools commented 9 months ago

Agree. I added it above.

I'm not sure when I will get to v2 but you could sponsor the effort to speed things up 😉

nununo commented 9 months ago

I'm not sure when I will get to v2 but you could sponsor the effort to speed things up 😉

I'd happily help but the system I'm currently working on blocks github 😢 If I manage to find an alternative I'll definitely contribute.

AlexandreHT commented 9 months ago

Ideally zif_logger shouldn't have anything to do with UI. So... I don't think methods like display_fullscreen should have a place here. Instead you could have, for example, zif_logger_ui implemented by zcl_logger_ui_fullscreen which receives zif_logger in its constructor.

Hear, hear. I'd propose helper methods into zif_logger_display such as display_if_has_warning (or worse), display_if_has_error (or worse). I'll believe we could decouple display class from abap_logger just by keeping the handler exposed as read-only in the interface.

@nununo, you can install the standalone version of abapGit, use offline projet, and move the file exported with abapGit to the folder wher you've cloned the projet (that's how I do it actually)

nununo commented 9 months ago

@AlexandreHT Great news! That's good to know. I was wondering if that would work. I'm glad it does. I also have a couple of open source projects which have been stalled because of that which I can work on again then.

nununo commented 9 months ago

SAP standard already has IF_RECA_MESSAGE_LIST which overlaps somewhat with ABAP Logger (https://abapinho.com/en/2023/01/message-collector/)

Imho, ABAP Logger v2 should not reinvent the wheel. It should build on top of it. I suggest that ABAP Logger v2 could be made as a wrapper around the existing IF_RECA_MESSAGE_LIST functionality so as to avoid duplication.

mbtools commented 9 months ago

We don't want to introduce new dependencies on standard SAP. It also needs to work for 702 and higher (and eventually on ABAP Cloud, which does not have if_reca...)

nununo commented 9 months ago

We don't want to introduce new dependencies on standard SAP. It also needs to work for 702 and higher (and eventually on ABAP Cloud, which does not have if_reca...)

Makes sense. Understood.