Adapt to Token Macro's new escapeHtml parameter #157
Conversation
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.hamcrest</groupId> | ||
| <artifactId>hamcrest-core</artifactId> | ||
| <version>2.2</version> | ||
| <!--<version>2.2</version>--> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
nit: I think this dependency already comes in transitively from jenkins-test-harness at this version. Can it be deleted?
There was a problem hiding this comment.
Yea, I just forgot to remove it after all my back and forth testing what I need to do to make the require upper bounds to be gone :)
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.jenkins-ci.main</groupId> | ||
| <artifactId>jenkins-test-harness</artifactId> | ||
| <version>1736.vc72c458c5103</version> |
There was a problem hiding this comment.
nit: Could the parent pom be updated instead? Also for what it's worth you can set the property <jenkins-test-harness.version> without having to declare the whole dependency.
| @@ -133,7 +146,11 @@ public String render(FailureCauseBuildAction action) { | |||
| final FailureCauseDisplayData data = action.getFailureCauseDisplayData(); | |||
| if (data.getFoundFailureCauses().isEmpty() && data.getDownstreamFailureCauses().isEmpty()) { | |||
| logger.info("there were no causes"); | |||
| return noFailureText; | |||
| if (!useHtmlFormat && escapeHtml) { | |||
There was a problem hiding this comment.
Would it make sense to use !useHtmlFormat || escapeHtml instead? Or maybe get rid of escapeHtml completely and just use useHtmlFormat to control escaping? Or in other words, if I set escapeHtml to true, would it make sense to set useHtmlFormat to true? Or would that be a pointless configuration? Same for setting escapeHtml to false and useHtmlFormat to false, would that be redundant?
(Maybe I'm just confused about the name useHtmlFormat.)
There was a problem hiding this comment.
escapeHtml is the parameter that I can't get rid of, it's now inherited from the parent class. useHtmlFormat is the one that has always been there and if it is true it will escape all relevant parts and always has. So the "new" parameter is then only relevant if the user wants a non html formatted list.
| @@ -297,9 +318,17 @@ private void addIndicationRepresentation(final StringBuilder stringBuilder, | |||
| stringBuilder.append(":\n"); | |||
| stringBuilder.append(indentForDepth(indentLevel)); | |||
| stringBuilder.append(LIST_BULLET_SPACE); | |||
| stringBuilder.append("<"); | |||
| if (escapeHtml) { | |||
| stringBuilder.append("<"); | |||
There was a problem hiding this comment.
If the plugin is adding this itself, is it always safe? Or can we not guarantee that? I'm confused about why we would emit < here vs just not emitting anything.
There was a problem hiding this comment.
I am interpreting the situation as the user wants the text version of the output, but still want it to be safe html 🤷♂️
…plugin into escapeHtmlTokenField
See jenkinsci/token-macro-plugin#118
Also added help documentation for the token macro.