Issue 116 Name and Reuse Repeating Conditions #131
Conversation
![sonarcloud[bot]](https://avatars.githubusercontent.com/in/12526?s=60&v=4){kind=small}
# Conflicts: # src/test/java/com/cognifide/aemrules/htl/HtlProfileTest.java
src/main/resources/rules/HTL-5.md
Outdated
| @@ -0,0 +1,25 @@ | |||
| Always Place HTL Attributes After the Ones that are Part of the Markup | |||
There was a problem hiding this comment.
This description is actually for different rule!
There was a problem hiding this comment.
changed to: Always try to re-use existing conditions, so the code is more readable.
src/main/java/com/cognifide/aemrules/htl/checks/NamingAndReusingConditionsCheck.java
Outdated
Show resolved
Hide resolved
src/main/java/com/cognifide/aemrules/htl/checks/NamingAndReusingConditionsCheck.java
Show resolved
Hide resolved
src/main/java/com/cognifide/aemrules/htl/checks/NamingAndReusingConditionsCheck.java
Show resolved
Hide resolved
src/main/java/com/cognifide/aemrules/htl/checks/NamingAndReusingConditionsCheck.java
Outdated
Show resolved
Hide resolved
src/main/java/com/cognifide/aemrules/htl/checks/NamingAndReusingConditionsCheck.java
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/java/com/cognifide/aemrules/htl/rules/HtlCheckClasses.java
|
I think that the previous implementation was better and needed a tweak. It's hard to understand current approach. |
|
|
||
| @Override | ||
| public void startHtlElement(List<Expression> expressions, TagNode node) { | ||
| if (isConditionReused(expressions, node)) { |
There was a problem hiding this comment.
This time also, the method name is misleading since "reused" has a positive connotation whereas in your case reused means "the same condition used couple of times needlessly".
There was a problem hiding this comment.
Conditions inverted and method name changed to isConditionReusedCorrectly().
| private Set<String> namedConditions = new HashSet<>(); | ||
|
|
||
| @Override | ||
| public void startHtlElement(List<Expression> expressions, TagNode node) { |
There was a problem hiding this comment.
This API has to be rewritten as it decouples expression from the attribute.
There was a problem hiding this comment.
@piotr-wilczynski You should take a look at this :)
There was a problem hiding this comment.
this method is giving You all of the expressions that exists in node, but if You want the have relation between attribute and expresion You can always check if there is any expression in the attribute and proceed somehow. There is a method called htlExpression default void htlExpression(Expression expression, Node node) It is probably what You are looking for @chutch ?
| private boolean isConditionReused(List<Expression> expressions, TagNode node) { | ||
| String condition = expressions.stream() | ||
| .map(Expression::getRawText) | ||
| .map(text -> text.replaceAll("[${}]", "")) |
There was a problem hiding this comment.
Please extract 'getRawText' and 'text.replaceAll("[${}]", "")' to method and reuse it in '103' line.
There was a problem hiding this comment.
Extracted
private Set<String> clearExpressions(List<Expression> expressions){ return expressions.stream() .map(Expression::getRawText) .map(text -> text.replaceAll("[${}]", "")) .collect(Collectors.toSet()); }
| .filter(text -> text.contains(SLY_TEST)) | ||
| .findFirst(); | ||
| if (condition.isPresent() && !condition.get().equals(SLY_TEST)) { | ||
| condition = Optional.of(condition.get().substring(14)); |
src/main/resources/rules/HTL-5.md
Outdated
| <!--/* Bad - the same condition is evaluated multiple times */--> | ||
| <span class="uber-mode__top-bar" data-sly-test="${uberModeHelper.uberModeEnabled || forceUberMode}"> | ||
| <div class="my-component"> | ||
| Blah blah blah |
| private Set<String> namedConditions = new HashSet<>(); | ||
|
|
||
| @Override | ||
| public void startHtlElement(List<Expression> expressions, TagNode node) { |
There was a problem hiding this comment.
this method is giving You all of the expressions that exists in node, but if You want the have relation between attribute and expresion You can always check if there is any expression in the attribute and proceed somehow. There is a method called htlExpression default void htlExpression(Expression expression, Node node) It is probably what You are looking for @chutch ?
| @ParsingErrorRule | ||
| public class NamingAndReusingConditionsCheck extends AbstractHtlCheck { | ||
|
|
||
| static final String RULE_KEY = "HTL-4"; |
There was a problem hiding this comment.
You define HTL-4 rule key, but doc file is named HTL-5, why?
# Conflicts: # README.md
fixes #116