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

Introduce ABAP Language Version (ALV) to .abapgit.xml #6335

Closed
wants to merge 6 commits into from
Closed

Introduce ABAP Language Version (ALV) to .abapgit.xml #6335

wants to merge 6 commits into from

Conversation

ThomasPloski
Copy link
Collaborator

fixes issue #6334

@ThomasPloski ThomasPloski changed the title Introduce ABAP language version to .abapgit.xml Introduce ABAP Language Version (ALV) to .abapgit.xml Jul 5, 2023
@mbtools
Copy link
Member

mbtools commented Jul 5, 2023

Code is part of the solution and looks good to me. However, I don't think we should add this as is. Every newly created repo will get diffs when used in older abapGit versions (not sure what happens if you try to pull).

We should mark the feature to be experimental so we can merge without impacting the community.

IF zcl_abapgit_persist_factory=>get_settings( )->read( )->get_experimental_features( ) = abap_true.
...
ENDIF.

@ThomasPloski
Copy link
Collaborator Author

I will surround the affected coding parts with the mentioned if statement and update the PR.

@mbtools mbtools added the new feature New feature or request label Jul 6, 2023
Remove default value for ABAP language version in .abapgit.xml
@ThomasPloski
Copy link
Collaborator Author

One step back: I have decided to not touch the current logic in .abapgit.xml handling, so that surrounding with experimental feature is not required, since there is no defaulting while object creation.

@mbtools
Copy link
Member

mbtools commented Jul 13, 2023

looks like these changes are included in #6346

Copy link
Collaborator

@schneidermic0 schneidermic0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ThomasPloski. LGTM

@ThomasPloski
Copy link
Collaborator Author

This PR can be closed since the PR #6346 contains of those changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

4 participants