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
fabianlupa opened this issue Jan 20, 2019 · 7 comments · Fixed by #2630
Closed

Force alphabetical method order #2321

fabianlupa opened this issue Jan 20, 2019 · 7 comments · Fixed by #2630
Assignees

Comments

@fabianlupa
Copy link
Member

fabianlupa 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
Copy link
Member

larshp commented Jan 21, 2019

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

@fabianlupa
Copy link
Member Author

fabianlupa 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
Copy link
Member

larshp 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
Copy link
Member

pokrakam 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.

@fabianlupa
Copy link
Member Author

fabianlupa 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
Copy link
Member

pokrakam 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
Copy link
Contributor

GoWale 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
Labels
None yet
4 participants