Skip to content

Commit

Permalink
fix AlignMethodsDeclarationRule for mixed chains (#13)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jmgrassau
jmgrassau committed May 22, 2023
1 parent 5194ffc commit d5f4dfc
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 19 deletions.
8 changes: 5 additions & 3 deletions ...ap.adt.abapcleaner/src/com/sap/adt/abapcleaner/rules/alignment/AlignDeclarationsRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,9 +488,11 @@ private Token readDeclarationLine(AlignLine line, Token token, int additionalInd
Token typeEnd = token;
while (typeEnd != null && !typeEnd.isAnyKeyword("LENGTH", "DECIMALS", "VALUE", "READ-ONLY") && !typeEnd.textEqualsAny(".", ",")) {
// do not align table declarations with "WITH ... KEY ..." sections, because they usually should not be put on a single line;
// however, do accept the short cases of "WITH EMPTY KEY" and "WITH [UNIQUE | NON-UNIQUE] DEFAULT KEY"
if (typeEnd.isKeyword("WITH") && !typeEnd.matchesOnSiblings(true, "WITH", "EMPTY", "KEY")
&& !typeEnd.matchesOnSiblings(true, "WITH", TokenSearch.makeOptional("UNIQUE|NON-UNIQUE"), "DEFAULT", "KEY")) {
// however, do accept the short cases of "WITH EMPTY KEY" and "WITH [UNIQUE | NON-UNIQUE] DEFAULT KEY" and "WITH [UNIQUE | NON-UNIQUE] KEY comp1"
if (typeEnd.isKeyword("WITH")
&& !typeEnd.matchesOnSiblings(true, "WITH", "EMPTY", "KEY")
&& !typeEnd.matchesOnSiblings(true, "WITH", TokenSearch.makeOptional("UNIQUE|NON-UNIQUE"), "DEFAULT", "KEY")
&& !typeEnd.matchesOnSiblings(true, "WITH", TokenSearch.makeOptional("UNIQUE|NON-UNIQUE"), "KEY", TokenSearch.ANY_IDENTIFIER, ",|.")) {
return null;
}
typeEnd = typeEnd.getNextSibling();
Expand Down
22 changes: 15 additions & 7 deletions ....abapcleaner/src/com/sap/adt/abapcleaner/rules/alignment/AlignMethodsDeclarationRule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ private boolean isDeclarationSuitedForMultiDeclarations(AlignTable table, int me

} else if (getAlignConsecutive() == MethodsSequenceAlignment.ALL_TABULAR) {
// allow one-liners as well as declarations in 'tabular' layout (or chains of those), i.e. ensure that there are no line breaks...
// - after the keywords METHODS / CLASS-METHODS,
// - after the keywords METHODS / CLASS-METHODS (except when they are followed by a chain colon)
// - after the method names, or
// - after the access keywords IMPORTING, EXPORTING etc.
final Columns[] columnsToCheck = new Columns[] { Columns.KEYWORD, Columns.METHOD_NAME, Columns.ACCESS };
Expand All @@ -434,7 +434,7 @@ private boolean isDeclarationSuitedForMultiDeclarations(AlignTable table, int me
if (cell == null)
continue;
Token nextToken = cell.getLastToken().getNextNonCommentSibling();
if (nextToken != null && nextToken.lineBreaks > 0) {
if (nextToken != null && nextToken.lineBreaks > 0 && !cell.getLastToken().isChainColon()) {
// line break found - this is no one-liner or 'tabular' layout
// or, as an exception, continue if (lineIndex == methodNameLineIndex && ChangeType.forValue(configContinueAfterKeyword.getValue()) == ChangeType.NEVER)?
return false;
Expand All @@ -458,8 +458,16 @@ private void executeOn(Code code, Command startCommand, AlignTable table, boolea
basicIndent += ABAP.INDENT_STEP;
int firstLineBreaks = table.getFirstToken().lineBreaks;

// determine whether the table starts or ends in the middle of a declaration chain
boolean tableContainsPartialChains = false;
if (table.getLine(0).getCell(Columns.KEYWORD.getValue()) == null) {
tableContainsPartialChains = true;
} else if (table.getLastLine().getLastToken().matchesOnSiblings(true, TokenSearch.ASTERISK, ABAP.COLON_SIGN_STRING)) {
tableContainsPartialChains = true;
}

// insert line breaks depending on rule configuration
changeLineBreaks(code, table, isTableOfOneLinersOrTabular, basicIndent);
changeLineBreaks(code, table, isTableOfOneLinersOrTabular && !tableContainsPartialChains, basicIndent);

// decide whether the DEFAULT or OPTIONAL column should really be aligned (i.e. whether horizontal space should be reserved for them):
// if only one single line (or less than 20% of all lines) have content in this column, then join the content into a previous column
Expand Down Expand Up @@ -495,8 +503,8 @@ private void executeOn(Code code, Command startCommand, AlignTable table, boolea
}
}

private void changeLineBreaks(Code code, AlignTable table, boolean isTableOfOneLinersOrTabular, int basicIndent) {
boolean isPotentialOneLiner = (table.getLineCount() == 1 && !table.getLine(0).containsComments());
private void changeLineBreaks(Code code, AlignTable table, boolean alwaysContinueLine, int basicIndent) {
boolean isPotentialOneLiner = (table.getLineCount() == 1 && !table.getLine(0).containsComments());
boolean isCurrentOneLiner = isPotentialOneLiner && !table.getLine(0).containsInnerLineBreaks();
MethodsOneLinerMeasure oneLinerMeasure = MethodsOneLinerMeasure.forValue(configHandleOneLiners.getValue());

Expand All @@ -508,8 +516,8 @@ private void changeLineBreaks(Code code, AlignTable table, boolean isTableOfOneL

// determine the configuration for this column
ChangeType continueLine;
if (isTableOfOneLinersOrTabular) {
continueLine = ChangeType.ALWAYS;
if (alwaysContinueLine) {
continueLine = (colIndex == Columns.KEYWORD.getValue()) ? ChangeType.KEEP_AS_IS : ChangeType.ALWAYS;
} else if (isPotentialOneLiner && oneLinerMeasure == MethodsOneLinerMeasure.CREATE) {
continueLine = ChangeType.ALWAYS;
} else if (isCurrentOneLiner && oneLinerMeasure == MethodsOneLinerMeasure.KEEP_EXISTING) {
Expand Down
5 changes: 3 additions & 2 deletions ...t.abapcleaner.test/src/com/sap/adt/abapcleaner/rules/alignment/AlignDeclarationsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,12 +672,13 @@ void testDataBeginOfNested() {

@Test
void testTableDeclarationWithKey() {
// ensure that table declarations with "WITH ... KEY ..." sections are not aligned, because they usually should not be put on a single line
// ensure that table declarations with "WITH ... KEY ..." sections are not aligned (except for very simply cases
// with just one component), because they usually should not be put on a single line

buildSrc(" DATA:");
buildSrc(" lt_item TYPE ty_tt_item,");
buildSrc(" lts_buffer TYPE SORTED TABLE OF ty_s_buffer");
buildSrc(" WITH NON-UNIQUE KEY primary_key,");
buildSrc(" WITH NON-UNIQUE KEY comp1 comp2 comp3,");
buildSrc(" lv_index TYPE i.");

copyExpFromSrc();
Expand Down
52 changes: 45 additions & 7 deletions ...cleaner.test/src/com/sap/adt/abapcleaner/rules/alignment/AlignMethodsDeclarationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1038,9 +1038,9 @@ void testOneLinersAfterChainWithComment() {
buildSrc(" method_with_long_name RETURNING VALUE(rv_third) TYPE string.");

buildExp(" METHODS: \" comment");
buildExp(" any_method RETURNING VALUE(rv_any) TYPE abap_bool,");
buildExp(" other_method RETURNING VALUE(rv_other) TYPE i,");
buildExp(" method_with_long_name RETURNING VALUE(rv_third) TYPE string.");
buildExp(" any_method RETURNING VALUE(rv_any) TYPE abap_bool,");
buildExp(" other_method RETURNING VALUE(rv_other) TYPE i,");
buildExp(" method_with_long_name RETURNING VALUE(rv_third) TYPE string.");

putAnyClassDefAroundSrcAndExp();

Expand Down Expand Up @@ -1780,24 +1780,62 @@ void testMethodsAbstract() {
testRule();
}

/*
@Test
void testBreakAfterKeyword() {
rule.configContinueAfterKeyword.setEnumValue(ChangeType.NEVER);
void testBreakAfterKeywordAlignOneLiners() {
rule.configContinueAfterMethodName.setEnumValue(ChangeType.ALWAYS);
rule.configAlignConsecutive.setEnumValue(MethodsSequenceAlignment.ONE_LINERS);

buildSrc(" METHODS:");
buildSrc(" any_method IMPORTING iv_any TYPE i,");
buildSrc(" other_method IMPORTING iv_other TYPE i,");
buildSrc(" third_method_with_long_name IMPORTING iv_third_param TYPE i.");
buildSrc("");
buildSrc(" DATA lv_any TYPE i.");

buildExp(" METHODS:");
buildExp(" any_method IMPORTING iv_any TYPE i,");
buildExp(" other_method IMPORTING iv_other TYPE i,");
buildExp(" third_method_with_long_name IMPORTING iv_third_param TYPE i.");
buildExp("");
buildExp(" DATA lv_any TYPE i.");

testRule();
}

@Test
void testBreakAfterKeywordAlignAllTabular() {
rule.configContinueAfterMethodName.setEnumValue(ChangeType.ALWAYS);
rule.configAlignConsecutive.setEnumValue(MethodsSequenceAlignment.ALL_TABULAR);

buildSrc(" METHODS:");
buildSrc(" any_method IMPORTING iv_any TYPE i,");
buildSrc(" other_method IMPORTING iv_other TYPE i,");
buildSrc(" third_method_with_long_name IMPORTING iv_third_param TYPE i.");
buildSrc("");
buildSrc(" DATA lv_any TYPE i.");

buildExp(" METHODS:");
buildExp(" any_method IMPORTING iv_any TYPE i,");
buildExp(" other_method IMPORTING iv_other TYPE i,");
buildExp(" third_method_with_long_name IMPORTING iv_third_param TYPE i.");
buildExp("");
buildExp(" DATA lv_any TYPE i.");

testRule();
}

@Test
void testBreakAfterKeywordButKeepOneLiners() {
rule.configContinueAfterKeyword.setEnumValue(ChangeType.NEVER);

buildSrc(" METHODS set_value IMPORTING !iv_new_value TYPE i.");
buildSrc(" METHODS get_current_value RETURNING VALUE(rv_result) TYPE i.");
buildSrc(" METHODS get_previous_value RETURNING VALUE(rv_result) TYPE i.");

buildExp(" METHODS set_value IMPORTING !iv_new_value TYPE i.");
buildExp(" METHODS get_current_value RETURNING VALUE(rv_result) TYPE i.");
buildExp(" METHODS get_previous_value RETURNING VALUE(rv_result) TYPE i.");

testRule();
}
*/
}

0 comments on commit d5f4dfc

Please sign in to comment.