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

Force alphabetical method order #2321

Closed
flaiker opened this issue Jan 20, 2019 · 7 comments

Comments

Projects
None yet
4 participants
@flaiker
Copy link
Collaborator

commented Jan 20, 2019

I am seeing many pull requests where the change is just a few lines and there are sometimes hundreds of other lines changed because of "SE80 method reordering".

Maybe this could be solved by an option in the repository settings. If it is enabled the class implementation part is build by abapGit, by appending each method implementation include in alphabetical order. Then regardless of if you are using ADT or the source-based SE24, the serialized class implementation should be the same.
I think a "wrong" method order in the repository's files is preferable to a polluted git history.

@larshp

This comment has been minimized.

Copy link
Owner

commented Jan 21, 2019

:trollface: problem is some are using Eclipse :trollface:

@flaiker

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 21, 2019

Idea would be that both ADT and SE80 serialize to the same .abap file regardless of the method order.
Of course there is no chance if someone decides to use the form based class editor and reformats the carefully handcrafted definition sections with the click of a button...

@larshp

This comment has been minimized.

Copy link
Owner

commented Jan 21, 2019

its difficult, parsing ABAP is a mess 😄

sometimes some of the editors also add "PROTECTED SECTION." if it is missing in the global class definition

@pokrakam

This comment has been minimized.

Copy link
Collaborator

commented Jan 22, 2019

I vote against this, my method ordering is deliberate*
It would be annoying if AG started to rearrange things. Yes it happens when someone edits it with SE80, but that's a lot rarer.

If anything, I'd say SE80 is at fault for not respecting it's own sequence definition.

*: Roughly in order of usage & dependency. Methods near where they're called from.

@flaiker

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 23, 2019

If anything, I'd say SE80 is at fault for not respecting it's own sequence definition.

It sure is, but we have no access to fix SE80.

I vote against this, my method ordering is deliberate*
It would be annoying if AG started to rearrange things. Yes it happens when someone edits it with SE80, but that's a lot rarer.

My point is that the method order is already broken in projects where you cannot force everyone to use ADT. So might as well make the order consistent across IDEs so that version control is not impacted (as an optional setting on repository level). Things like git blame are borderline unusable if 90% of the changes in a commit are totally unrelated method order changes.
(Also as far as I know even ADT still has the bug that reorders methods on activation sometimes (link). So in my experience getting the deliberate order of methods in the implementation part of a global class to stay that way is just a matter of luck.)

If there's no interest I can close the issue. I just get sad looking at the pull requests on this project with every second one having the comment "rest is reordering by SE80".

@pokrakam

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2019

I do agree with your sentiment, maybe someone should log this on OSS (unfortunately I’m not in a position to). SAP can be quite responsive, and this is a legitimate issue that affects all developers regardless of abapGit.

But to make it a configurable option and build the reordering just seems a lot of effort that I feel can be put to better use.

I think there are human solutions to this, if I’m working on a public project I need to take this into account and evaluate the likelihood of someone “doing an SE80” on my work, but for my private projects I have no interference. Maybe the project owner should exercise some control - if I’m in that position I’d either do an “SE80 reorder” before merging or request the author to submit it as two requests.

@GoWale

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2019

As alternative the format for abap class could be changed.
To split a class in all it parts:

  • Header Public/Proteced/private
  • Methods
  • etc.
    so it will kind the same as with function groups
    This will not solve the issue but the order of the methods will only conflict with the header parts?
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.