Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add repo option to force old serializer for CLAS #2491

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
2 participants
@flaiker
Copy link
Collaborator

commented Mar 10, 2019

Fixes #2321

I would still like to have this option available to prevent conflicts when using branches (i. e. transport = branch). Mainly to prevent this bug to have any effect in abapGit (#756).

I noticed that the "old" serializer seems to create the result when stuff gets rearranged in ADT, which is already used by abapGit as a fallback if the new one is not available. So I just added an option to always use it instead for CLAS.

The approach I chose to inject the read access to the repo setting in ZCL_ABAPGIT_OO_BASE is not very pretty. I could not find any existing dependencies from OBJECT_* classes to REPO so I am not sure how to handle it in a nice way.

rt_source = serialize_abap_old( is_class_key ).
RETURN.
ENDIF.

This comment has been minimized.

Copy link
@larshp

larshp Mar 10, 2019

Owner

suggest refactoring method, same method is called twice

rv_force_method_order = abap_false.
##TODO.
" get_repo_by_object or get_repo_by_package would be nice...
TRY.

This comment has been minimized.

Copy link
@larshp

larshp Mar 10, 2019

Owner

this method seems strange/misplaced, and I guess it will impact performance on all CLAS serialization?

This comment has been minimized.

Copy link
@flaiker

flaiker Mar 10, 2019

Author Collaborator

Yea, I dislike it too.
Do you have a suggestion on how to pass the flag through to serialize_abap_clif_source? Just add a parameter through the whole callstack? Or add another constructor parameter for CLAS and special handling in ZCL_ABAPGIT_OBJECTS->CREATE_OBJECT ?

This comment has been minimized.

Copy link
@flaiker

flaiker Mar 10, 2019

Author Collaborator

Hmm, I moved stuff around a bit. I think it's better than before, but not sure if acceptable.

@larshp

This comment has been minimized.

Copy link
Owner

commented Mar 10, 2019

Suggest renaming the option to "use_old_clas_serializer"

@larshp

This comment has been minimized.

@larshp

This comment has been minimized.

Copy link
Owner

commented Mar 11, 2019

I dont like the overall approach to passing the parameter, it breaks isolation, but currently I dont have any better suggestions

@flaiker

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 16, 2019

Just fyi, the function module name gets saved that way in ADT and you cannot change it. I'll change it back in SE37 once the strategy is clear.

I guess the options are the following:

  1. Read the additional repo setting in the object class

    • This breaks isolation, adds a dependency from OBJECT_* to repo.
  2. Inject the option at OBJECT_* creation

    • This is the current approach in this PR.
    • A more future proof version of this might be to replace iv_force_old_serializer in zcl_abapgit_object_clas=>constructor by an is_options structure so that it can be extended in the future without changing constructor parameters. The structure would be defined globally (for example in zif_abapgit_definitions. In zcl_abapgit_objects->create_object we could instead of looking for specific implementations of zif_abapgit_object wrap the constructor call with is_options in another try catch block for cx_sy_create_object_error and if it fails fall back to the constructor with only is_item and iv_language.
    • Could also add a method zif_abapgit_object~set_options that gets called after object creation to do the same thing. Then all object classes will have to be adjusted though.
  3. Inject the option in zif_abapgit_object~serialize

    • A different approach would be to add an optional parameter to serialize, then object creation would not have to be touched.
    • I haven't looked into this yet, not sure if it's more or less effort.

In my opinion it should be possible to give the OBJECT_* classes some additional settings based on the repository settings. I'd favor the structure approach described in 2, as it could also be used for things other than serialization in the future, should the need arise.

@larshp

This comment has been minimized.

Copy link
Owner

commented Mar 16, 2019

is there any way to manually force reordering? An alternative approach could be fixing this with IDE and CI

@flaiker

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 16, 2019

Not in ADT as far as I know. If you use the source based class editor in SE24 it automatically sorts the method implementations and leaves the definition sections alone I think? Form based SE24 will also reorganize everything in the definitions. There's also this "reorganize sections" button somewhere in the menu bar. Will check on monday.

Do you think this problem should not be solved by abapGit?

@larshp

This comment has been minimized.

Copy link
Owner

commented Mar 17, 2019

Just exploring options, there is some overlap in git/editor/CI setup here. Plus from an implementation point of view, the solution does not fit very nicely into abapGit, due to the architecture.

The problem we are trying to solve is avoiding diffs after import of classes, and there is no diff when methods are sorted alphabetically.

If I implement a new method using vscode(yeah, I'm crazy), then the same problem would occur. So another approach could be implementing a check for method sorting in abaplint which would run as a CI task. Plus a code inspector check to help the SE24/ADT user, perhaps add a keyboard shortcut in Eclipse.

I dont like having the git client change sources(but sometimes it does anyhow), ideally, it should show the same source as the developer entered. But again, everything has to fit together end to end.

@larshp

This comment has been minimized.

Copy link
Owner

commented Apr 14, 2019

What do you think about implementing(I can do this if you like) a check for method sorting in abaplint which would run as a CI task, this would also solve the overall issue?

@flaiker

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 14, 2019

Well, let's see:

Current behavior

  1. Developer creates transport
  2. Developer changes a method, activates class
  3. The bug occurs which causes reordering of the whole CLASS IMPLEMENTATION part
  4. Developer creates a branch for the transport in abapGit
  5. Developer commits changes
  6. Developer creates a pull request
  7. Reviewing the PR is close to impossible because there may be hundreds of lines changed that do not belong to the changes that were actually made.
  8. When merged the history for the class file includes these "non-changes" making git blame (and possibly lots of other git features) useless.

Force old class serializer

  1. Developer creates transport
  2. Developer changes a method, activates class
  3. The bug occurs which causes reordering of the whole CLASS IMPLEMENTATION part
  4. Developer creates a branch for the transport in abapGit
  5. Developer commits changes
  6. Developer creates a pull request
  7. Reviewing the PR is possible, because it only contains the changes the developer made even though the bug occurred.
  8. When merged the history is fine.
  9. Only thing is that in git the class files likely have a different IMPLEMENTATION part than in the development system. Which is not a problem in my case, because there only is one development system and the direction is always SAP -> git.

abaplint check

  1. Developer creates transport
  2. Developer changes a method, activates class
  3. The bug occurs which causes reordering of the whole CLASS IMPLEMENTATION part
  4. Developer creates a branch for the transport in abapGit
  5. Developer commits changes
  6. abaplint method order check runs, does not fail because the bug reordered everything / everything was already sorted
  7. Developer creates a pull request
  8. Reviewing the PR is possible, because it only contains the changes the developer made.
  9. When merged the history is fine.

abaplint check2

  1. Developer creates transport
  2. Developer creates a class, implements multiple methods, does not alphabetically sort methods by name, activates class
  3. The bug does not occur
  4. Developer creates a branch for the transport in abapGit
  5. Developer commits changes
  6. abaplint method order check runs, fails, alerts the developer
  7. The developer has to go into the class again and manually reorder the method implementations (?)
  8. Developer commits changes
  9. Developer creates a pull request
  10. Reviewing the PR is possible, because it only contains the changes the developer made.
  11. When merged the history is kind of fine? I guess if you squash the commits it is as if nothing happened, though I usually dislike squashing.

Sure, the abaplint solution would be the way to go to check for the problem in an environment where changes come from multiple "unconnected" SAP systems like in the open source world or where changes come from other editors (even using the web interface in GitHub).
For an on premise, one development system, multiple developers type of situation I can skip the manual reordering step by using the force old serializer approach. So that would still be the way to go for me.

If this is not something that belongs in abapGit, I am also happy to just maintain a modification that changes like 3 lines of code to use the old serializer for all classes within our namespace.

(And just to reiterate: If SAP would fix the bug so that the code you type in the editor is also the one that's saved, this problem would not exist.)

@larshp

This comment has been minimized.

Copy link
Owner

commented Apr 14, 2019

"3 lines of code", we can easily add a user exit in METHOD serialize_abap_clif_source, no problem, this would also be easier for you, as there would be no modifications to the abapGit code. Plus few overall changes to abapGit itself

@flaiker

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 14, 2019

Sounds good to me!
Closing in favor of user exit approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.