You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by dawidwys <gi...@git.apache.org> on 2018/07/11 17:19:59 UTC
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
GitHub user dawidwys opened a pull request:
https://github.com/apache/flink/pull/6312
[FLINK-9792] Added custom Description class for ConfigOptions to enable rich formatting.
## What is the purpose of the change
Enable rich formatting of options description.
## Brief change log
*(for example:)*
- Implemented Description class that represents description with formatting.
- added html formatter that transforms Description to Html string
## Verifying this change
This change added tests and can be verified as follows:
- DescriptionHtmlTest
- all tests for generating documentation still works
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (not applicable / docs / **JavaDocs** / not documented)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dawidwys/flink FLINK-9792
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6312.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #6312
----
commit 96a19555539fcb24122bb4f5efb346dc8fa30db9
Author: Dawid Wysakowicz <dw...@...>
Date: 2018-07-11T07:52:48Z
[FLINK-9792] Added custom Description class for ConfigOptions to enable rich formatting.
----
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201805835
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/ListElement.java ---
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Represents a list in the {@link Description}.
+ */
+public class ListElement implements BlockElement {
+
+ private final List<TextElement> entries;
--- End diff --
This should allow any in-line element imo, why can't we have a list of links?
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203069177
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
--- End diff --
any my point is exactly to prevent bugs from masking each other. The placeholder was safe to use since we could be reasonably sure it will not be present in the input. Once 2 entities use the same placeholder, with access to the same input, this is no longer the case.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203268482
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
Actually I think it would make even more sense if I move it to `TextElement`
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202992096
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
Sure, it is problematic whenever we use the '%' symbol for percents e.g.
Description description = Description.builder()
.text("This is a text that has some percentage value of 20%.")
.build();
There are already some descriptions that break it e.g
>The relative amount of memory (after subtracting the amount of memory used by network buffers) that the task manager reserves for sorting, hash tables, and caching of intermediate results. For example, a value of `0.8` means that a task manager reserves 80% of its memory for internal data buffers, leaving 20% of free memory for the task manager's heap for objects created by user-defined functions. This parameter is only evaluated, if taskmanager.memory.size is not set.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201932360
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
+ return formatter.format(this);
--- End diff --
this was only a rough draft, you could have inlineElement always return empty lists for getChildren and blockelement override getValue() and mark those as final. Finally `getChildren` could be changed to only return `InlineElement`, voila no infinite nesting. (see below for an updated draft)
Note that your approach currently has infinite nesting capabilities due to `Text`, which accepts a number of `InlineElements`, which again can be `Text`. (hence why i added the separate Sequence type)
The whole idea here is to _not_ allow elements to do super custom stuff. If we want to shorten links for the documentation we could have the formatter always show them as `here`.
```
public enum ElementType {
TEXT,
LINK,
LIST,
LINE_BREAK,
ARRAY,
SEQUENCE
}
public interface Element {
String getValue()
List<InlineElement> getChildren()
ElementType getType()
}
class InlineElement implements Element {
@Override
final List<InlineElement> getChildren() {
return Collections.emptyList;
}
}
class BlockElement implements Element {
@Override
final String getValue() {
return null;
}
}
class HtmlFormatter implements Formatter {
format(Description description) {
description.getElements().stream()
.forEach(this::format)
.collect(Collectors.joining())
}
String format(Element element) {
switch (element.getType()) {
case TEXT:
return element.getValue()
case LIST:
StringBuilder sb = new StringBuilder()
for (Element item : element.getChildren()) {
sb.append(<whatver html/formatting prefix you use for lists>)
sb.append(format(item))
sb.append(<whatver html/formatting suffix you use for lists>)
}
return sb.toString()
case LINK:
return "<a href=" + element.getValue + ">"
case LINE_BREAK:
reutrn "<br>"
case SEQUENCE:
StringBuilder sb = new StringBuilder()
for (Element item : element.getChildren()) {
sb.append(format(item))
}
return sb.toString()
}
}
}
class Link extends InlineElement {
private final String value
Link(String text) {
this.value = text
}
getValue() {
return this.value
}
getType() {
return LINK
}
}
class Text extends InlineElement {
private final String value
Text(String text) {
this.value = text
}
getValue() {
retu this.value;
}
getType() {
return TEXT
}
}
class List extends BlockElement {
private final List<InlineElement> elements
List(InlineElement ... elements) {
this.elements = Arrays.asList(elements)
}
getChildren() {
return elements
}
getType() {
return LIST
}
}
class LineBreak extends InlineElement {
getValue() {
return null
}
getType() {
return LINE_BREAK
}
}
class ComboArrayWhatever extends BlockElement {
private final List<InlineElement> elements
List(InlineElement ... elements) {
this.elements = Arrays.asList(elements)
}
getChildren() {
return Collections.emptyList
}
getType() {
return SEQUENCE
}
}
```
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201802349
--- Diff: docs/_includes/generated/security_configuration.html ---
@@ -10,7 +10,7 @@
<tr>
<td><h5>security.ssl.algorithms</h5></td>
<td style="word-wrap: break-word;">"TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"</td>
- <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
+ <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
--- End diff --
it shouldn't be escaped at all, no?
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202362348
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
--- End diff --
I could accept this more easily if we wouldn't return something as basic as a String. As it stands the element can circumvent whatever logic the `Formatter` has defined.
Could the `formatter` not maintain an internal state that is modified by the `format` calls, and change this method to return nothing?
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202359797
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
+ return formatter.format(this);
--- End diff --
Agree, it does not make sense. The reason why `Text` implements both `BlockElement` and `InlineElement` was to use `InlineElement` as entry in `ListElement`. This change allowed `ListElement` to contain both list and text.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203074481
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
ok that makes sense, do we maybe want to handle this in `Formatter´ already?.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201803366
--- Diff: docs/_includes/generated/security_configuration.html ---
@@ -10,7 +10,7 @@
<tr>
<td><h5>security.ssl.algorithms</h5></td>
<td style="word-wrap: break-word;">"TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"</td>
- <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
+ <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
--- End diff --
Aaa, that's what you meant. I haven't migrated the actual descriptions to use the new feature yet. Wanted to do it a separate commit/PR. First wanted to establish the API/feature.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201805424
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/Formatter.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Allows providing multiple formatters for the description. E.g. Html formatter, Markdown formatter etc.
+ */
+public abstract class Formatter {
+
+ /**
+ * Formats the description into a String using format specific tags.
+ *
+ * @param description description to be formatted
+ * @return string representation of the description
+ */
+ public String format(Description description) {
+ return description.getBlocks().stream().map(b -> b.format(this)).collect(Collectors.joining());
--- End diff --
the implementation here is rather weird.
First we give the formatter a description to format, which then passed itself to each block, which then again call into the formatter.
Why aren't we doing this with a simple overloaded `format` method for specific implementations, and then simply drill down through the description? (LinkElement, LineBreak, etc.)
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203305919
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/Formatter.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Allows providing multiple formatters for the description. E.g. Html formatter, Markdown formatter etc.
+ */
+public abstract class Formatter {
+
+ private StringBuilder state = new StringBuilder();
--- End diff --
could be final
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202385019
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
--- End diff --
yes that's what i meant.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203292416
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
Forget my comment, it should be done around formatting.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201819026
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/ListElement.java ---
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Represents a list in the {@link Description}.
+ */
+public class ListElement implements BlockElement {
+
+ private final List<TextElement> entries;
--- End diff --
True, changed it.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203003509
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
--- End diff --
Does it matter? Technically it does not go out of this escaping.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/6312
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202969620
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
--- End diff --
this should be distinct from the placeholder in `Utils`.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202599741
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
--- End diff --
Hmmm, I tried to implement the Formatter using internal state, but the only way it could be done(I think) would be with some stacking logic, which I think we could not generalize. We would need to implement it each time for a new Formatter. Therefore I don't think it is a good idea.
Would you be ok, if we do it with a switch, but rather than using enum, with `instanceof`s and casting? I would really prefer to have a specialized fields rather than a generic ones (like e.g. `value`).
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201818972
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/Formatter.java ---
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+import java.util.List;
+import java.util.stream.Collectors;
+
+/**
+ * Allows providing multiple formatters for the description. E.g. Html formatter, Markdown formatter etc.
+ */
+public abstract class Formatter {
+
+ /**
+ * Formats the description into a String using format specific tags.
+ *
+ * @param description description to be formatted
+ * @return string representation of the description
+ */
+ public String format(Description description) {
+ return description.getBlocks().stream().map(b -> b.format(this)).collect(Collectors.joining());
--- End diff --
Originally I thought this way there will be less loops in the implementation of concrete formatters, but I had a second look and I think it is not the case. I agree with your suggestion. In other words I unnecessary overcomplicated things. :(
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201806215
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/BlockElement.java ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Part of description that represents a block e.g. some text, linebreak or a list.
+ */
+public interface BlockElement {
+
+ /**
+ * Transforms itself into String representation using given format.
+ *
+ * @param formatter formatter to use.
+ * @return string representation
+ */
+ String format(Formatter formatter);
--- End diff --
This class is effectively identical to `InlineElement`; at the very least this method could be moved into a shared parent class.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203306483
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/Formatter.java ---
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Allows providing multiple formatters for the description. E.g. Html formatter, Markdown formatter etc.
+ */
+public abstract class Formatter {
+
+ private StringBuilder state = new StringBuilder();
+
+ /**
+ * Formats the description into a String using format specific tags.
+ *
+ * @param description description to be formatted
+ * @return string representation of the description
+ */
+ public String format(Description description) {
+ for (BlockElement blockElement : description.getBlocks()) {
+ blockElement.format(this);
+ }
+ return finalizeFormatting();
+ }
+
+ public void format(LinkElement element) {
+ formatLink(state, element.getLink(), element.getText());
+ }
+
+ public void format(TextElement element) {
+ String[] inlineElements = element.getElements().stream().map(el -> {
+ Formatter formatter = newInstance();
+ el.format(formatter);
+ return formatter.finalizeFormatting();
+ }
+ ).toArray(String[]::new);
+ formatText(state, escapeFormatPlaceholder(element.getFormat()), inlineElements);
+ }
+
+ public void format(LineBreakElement element) {
+ formatLineBreak(state);
+ }
+
+ public void format(ListElement element) {
+ String[] inlineElements = element.getEntries().stream().map(el -> {
+ Formatter formatter = newInstance();
+ el.format(formatter);
+ return formatter.finalizeFormatting();
+ }
+ ).toArray(String[]::new);
+ formatList(state, inlineElements);
+ }
+
+ private String finalizeFormatting() {
+ String result = state.toString();
+ state.setLength(0);
+ return result.replaceAll("%%", "%");
--- End diff --
If a link description contains `%%` this will unintentionally modify that, correct? Granted this is quite an edge-case.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203069199
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
and what is the actual issue?
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201819059
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/BlockElement.java ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Part of description that represents a block e.g. some text, linebreak or a list.
+ */
+public interface BlockElement {
+
+ /**
+ * Transforms itself into String representation using given format.
+ *
+ * @param formatter formatter to use.
+ * @return string representation
+ */
+ String format(Formatter formatter);
--- End diff --
Added a common parent.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201844991
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
+ return formatter.format(this);
--- End diff --
It is a basic Visitor Pattern approach. This way we have a typesafe way of implementing it. With the enum approach we would need to do casting if we have any custom parameters e.g. in link where beside link we also want to have a visible name for the link.
Also I would be in favour of splitting into inline and block elements to control complexity of the structure. I don't think we should support highly nested structures just to minimize future efforts.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202644607
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
--- End diff --
hmm... i think we can do this with internal state. I gave it a shot and came up with 2 versions:
1) internal state kept in `HtmlFormatter`
The internal state is a simple Stringbuilder. The only tricky part is the format string for `text()`, for this i create a new `HtmlFormatter` for each individual element, and then add them to the actual state.
Changes are isolated to `HtmlFormatter`.
2) internal state kept in `Formatter`, main formatting logic now resides in `Formatter` with hooks for specific elements (like List prefixes) that subclasses implement.
Same approach for dealing with `text()` as for 1), solved with `Formatter#duplicate()` method.
## 1)
```
public abstract class Formatter {
/**
* Formats the description into a String using format specific tags.
*
* @param description description to be formatted
* @return string representation of the description
*/
public String format(Description description) {
description.getBlocks().forEach(b -> b.format(this));
return finalizeFormatting();
}
public abstract String finalizeFormatting();
public abstract void format(LinkElement element);
public abstract void format(TextElement element);
public abstract void format(LineBreakElement element);
public abstract void format(ListElement element);
}
public class HtmlFormatter extends Formatter {
private final StringBuilder state = new StringBuilder();
public String finalizeFormatting() {
return state.toString();
//TODO: clean state
}
@Override
public void format(LinkElement element) {
state.append(String.format("<a href=\"%s\">%s</a>", element.getLink(), element.getText()));
}
@Override
public void format(TextElement element) {
String format = escapeCharacters(element.getFormat());
final List<InlineElement> inlineElements = element.getElements();
if (inlineElements.isEmpty()) {
state.append(format);
} else {
Object[] formattedInlineElements = new String[inlineElements.size()];
for (int x = 0; x < formattedInlineElements.length; x++) {
HtmlFormatter innerFormatter = new HtmlFormatter();
inlineElements.get(x).format(innerFormatter);
formattedInlineElements[x] = innerFormatter.finalizeFormatting();
}
state.append(String.format(format, formattedInlineElements));
}
}
@Override
public void format(LineBreakElement element) {
state.append("<br/>");
}
@Override
public void format(ListElement element) {
state.append("<ul>");
for (InlineElement item : element.getEntries()) {
state.append("<li>");
item.format(this);
state.append("</li>");
}
state.append("</ul>");
}
private static String escapeCharacters(String value) {
return value
.replaceAll("<", "<")
.replaceAll(">", ">");
}
}
## 2)
```
public abstract class Formatter {
private final StringBuilder state = new StringBuilder();
/**
* Formats the description into a String using format specific tags.
*
* @param description description to be formatted
* @return string representation of the description
*/
public String format(Description description) {
description.getBlocks().forEach(b -> b.format(this));
return finalizeFormatting();
}
public String finalizeFormatting() {
return state.toString();
}
public void format(LinkElement element) {
state.append(getLinkPrefix() + element.getLink() + getLinkInfix() + element.getText() + getLinkSuffix());
}
public abstract String getLinkPrefix();
public abstract String getLinkInfix();
public abstract String getLinkSuffix();
public void format(TextElement element) {
String format = escapeTextCharacters(element.getFormat());
final List<InlineElement> inlineElements = element.getElements();
if (inlineElements.isEmpty()) {
state.append(format);
} else {
Object[] formattedInlineElements = new String[inlineElements.size()];
for (int x = 0; x < formattedInlineElements.length; x++) {
Formatter innerFormatter = duplicate();
inlineElements.get(x).format(innerFormatter);
formattedInlineElements[x] = innerFormatter.finalizeFormatting();
}
state.append(String.format(format, formattedInlineElements));
}
}
public void format(LineBreakElement element) {
state.append(createLineBreak());
}
public abstract String createLineBreak();
public void format(ListElement element) {
state.append(getListPrefix());
for (InlineElement item : element.getEntries()) {
state.append(getListItemPrefix());
item.format(this);
state.append(getListItemSuffix());
}
state.append(getListSuffix());
}
public abstract String getListPrefix();
public abstract String getListSuffix();
public abstract String getListItemPrefix();
public abstract String getListItemSuffix();
public abstract Formatter duplicate();
protected abstract String escapeTextCharacters(String value);
}
public class HtmlFormatter extends Formatter {
@Override
public String getLinkPrefix() {
return "<a href=\"";
}
@Override
public String getLinkInfix() {
return "\">";
}
@Override
public String getLinkSuffix() {
return "</a>";
}
@Override
public String createLineBreak() {
return "<br/>";
}
@Override
public String getListPrefix() {
return "<ul>";
}
@Override
public String getListSuffix() {
return "</ul>";
}
@Override
public String getListItemPrefix() {
return "<li>";
}
@Override
public String getListItemSuffix() {
return "</li>";
}
protected String escapeTextCharacters(String value) {
return value
.replaceAll("<", "<")
.replaceAll(">", ">");
}
@Override
public Formatter duplicate() {
return new HtmlFormatter();
}
}
```
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201787098
--- Diff: docs/_includes/generated/security_configuration.html ---
@@ -10,7 +10,7 @@
<tr>
<td><h5>security.ssl.algorithms</h5></td>
<td style="word-wrap: break-word;">"TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"</td>
- <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
+ <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
--- End diff --
this doesn't look correct.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201803766
--- Diff: docs/_includes/generated/security_configuration.html ---
@@ -10,7 +10,7 @@
<tr>
<td><h5>security.ssl.algorithms</h5></td>
<td style="word-wrap: break-word;">"TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"</td>
- <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
+ <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
--- End diff --
Should've added comment to the PR.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201802591
--- Diff: docs/_includes/generated/security_configuration.html ---
@@ -10,7 +10,7 @@
<tr>
<td><h5>security.ssl.algorithms</h5></td>
<td style="word-wrap: break-word;">"TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"</td>
- <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
+ <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
--- End diff --
since it's part of a link
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202803069
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
--- End diff --
That's actually a really neat idea with the new instance of the formatter! I didn't think of this. It actually solves the part I had problems with.
I uploaded a slightly adjusted version of your second solution.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201940611
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
+ return formatter.format(this);
--- End diff --
As for the infinite nesting I could do exactly the same with the Visitor Pattern approach and introduce `Sequence` class. Both approaches are exactly the same in case of class hierarchy. It is just the question of how do we do a pattern matching - formatting. In languages with pattern matching on class type it is straightforward and we would go for a switch that would do a safe type inference.
In java we have two options Visitor pattern(my suggestion), switch on enum(your suggestion). I feel we are down to personal preference on that matter.
Why do I prefer the first one is that the classes are made to measure. They reflect structure of element: no empty methods, if unnecessary - e.g. no `getValue` in `LineBreak`, no `getChildren` methods in `InlineElements`, can have specialized fields in elements e.g. description for link, maybe importance for heading (h1-h6), could add number of line breaks into a single element.
I am afraid we end up in a preference lockdown.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r203070220
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
That the `% o` is considered to be placeholder by `String.format(` therefore in this particular case `MissingFormatArgumentException` is thrown.
---
[GitHub] flink issue #6312: [FLINK-9792] Added custom Description class for ConfigOpt...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on the issue:
https://github.com/apache/flink/pull/6312
Merging, will fix a spelling mistake in test while merging (if my travis gives green).
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202967561
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
@zentol Could you have a last look on this block code, before I merge it? Didn't want to sneak it in. I found out there is an issue when we use '%' sign in the description.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202970121
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/HtmlFormatter.java ---
@@ -0,0 +1,67 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Formatter that transforms {@link Description} into Html representation.
+ */
+public class HtmlFormatter extends Formatter {
+
+ @Override
+ protected void formatLink(StringBuilder state, String link, String description) {
+ state.append(String.format("<a href=\"%s\">%s</a>", link, description));
+ }
+
+ @Override
+ protected void formatLineBreak(StringBuilder state) {
+ state.append("<br/>");
+ }
+
+ @Override
+ protected void formatText(StringBuilder state, String format, String[] elements) {
+ String escapedFormat = escapeCharacters(format);
+ state.append(String.format(escapedFormat, elements));
+ }
+
+ @Override
+ protected void formatList(StringBuilder state, String[] entries) {
+ state.append("<ul>");
+ for (String entry : entries) {
+ state.append(String.format("<li>%s</li>", entry));
+ }
+ state.append("</ul>");
+ }
+
+ @Override
+ protected Formatter newInstance() {
+ return new HtmlFormatter();
+ }
+
+ private static final String TEMPORARY_PLACEHOLDER = "superRandomTemporaryPlaceholder";
+
+ private static String escapeCharacters(String value) {
+ return value
+ .replaceAll("%s", TEMPORARY_PLACEHOLDER)
--- End diff --
can you give me an example for where this is problematic? Does this occur if the _final formatted_ description contains `%`?
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201838689
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
+ return formatter.format(this);
--- End diff --
hmm..................................................................
Alright so this works but I still think it's kinda weird that the element calls into the formatter; this effectively gives the element control over how it is formatted, since it _could_ just return an arbitrary string.
Let me sketch out an alternative, the general idea being to generalize the parent interface (DescriptionElement) to accommodate all sub-classes, and introduce an enum for categorization.
With this the elements are just a container for data, and most (but not all logic unfortunately unless we go for instanceof checks) formatting logic is in the formatter.
```
public enum ElementType {
TEXT,
LINK,
LIST,
LINE_BREAK,
SEQUENCE // replaces your nested Text constructor
}
public interface Element {
String getValue()
List<Element> getChildren()
ElementType getType()
}
class HtmlFormatter implements Formatter {
format(Description description) {
description.getElements().stream()
.forEach(this::format)
.collect(Collectors.joining())
}
String format(Element element) {
switch (element.getType()) {
case TEXT:
return element.getValue()
case LIST:
StringBuilder sb = new StringBuilder()
for (Element item : element.getChildren()) {
sb.append(<whatver html/formatting prefix you use for lists>)
sb.append(format(item))
sb.append(<whatver html/formatting suffix you use for lists>)
}
return sb.toString()
case LINK:
return "<a href=" + element.getValue + ">"
case LINE_BREAK:
reutrn "<br>"
case SEQUENCE:
StringBuilder sb = new StringBuilder()
for (Element item : element.getChildren()) {
sb.append(format(item))
}
return sb.toString()
}
}
}
class Link implements Element {
private final String value
Link(String text) {
this.value = text
}
getValue() {
return this.value
}
getChildren() {
return Collections.emptyList()
}
getType() {
return LINK
}
}
class Text implements Element {
Text(String text) {
this.value = text
this.children = Collections.emptyList()
}
getValue() {
return this.value
}
getChildren() {
return Collections.emptyList
}
getType() {
return TEXT
}
}
class List implements Element {
private final List<Element> elements
List(Element ... elements) {
this.elements = Arrays.asList(elements)
}
getValue() {
return null
}
getChildren() {
return elements
}
getType() {
return LIST
}
}
class LineBreak implements Element {
getValue() {
return null
}
getChildren() {
return Collections.emptyList
}
getType() {
return LINE_BREAK
}
}
class ComboArrayWhatever implements Element {
private final List<Element> elements
List(Element ... elements) {
this.elements = Arrays.asList(elements)
}
getValue() {
return null
}
getChildren() {
return Collections.emptyList
}
getType() {
return SEQUENCE
}
}
```
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202365404
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
--- End diff --
You mean that `formatter` could *do* maintain internal state, right? If so, yes I can adjust it like that.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by dawidwys <gi...@git.apache.org>.
Github user dawidwys commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201796123
--- Diff: docs/_includes/generated/security_configuration.html ---
@@ -10,7 +10,7 @@
<tr>
<td><h5>security.ssl.algorithms</h5></td>
<td style="word-wrap: break-word;">"TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"</td>
- <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
+ <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
--- End diff --
Why do you think so? > and < are valid html encodings(I think it is even more common way of escaping < and >). I've changed it, cause it was impossible to retrieve the other version from `Jsoup` which is used in tests. I've also checked it renders well both in Chrome as well as in Firefox.
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r202355843
--- Diff: flink-core/src/main/java/org/apache/flink/configuration/description/LineBreakElement.java ---
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.configuration.description;
+
+/**
+ * Represents a line break in the {@link Description}.
+ */
+public class LineBreakElement implements BlockElement {
+
+ /**
+ * Creates a line break in the description.
+ */
+ public static LineBreakElement linebreak() {
+ return new LineBreakElement();
+ }
+
+ private LineBreakElement() {
+ }
+
+ @Override
+ public String format(Formatter formatter) {
+ return formatter.format(this);
--- End diff --
yes, nesting is an orthogonal issue.
For the nesting I suggest to change `Text` to implement `BlockElement` instead; it then effectively behaves like `Sequence`. At the moment i don't see a use-case for inserting Text into the format String since it could just be part of the format string.
I mean, when would this make sense?
```
text("hello %s", text("world"))
``
---
[GitHub] flink pull request #6312: [FLINK-9792] Added custom Description class for Co...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6312#discussion_r201803134
--- Diff: docs/_includes/generated/security_configuration.html ---
@@ -10,7 +10,7 @@
<tr>
<td><h5>security.ssl.algorithms</h5></td>
<td style="word-wrap: break-word;">"TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384"</td>
- <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
+ <td>The comma separated list of standard SSL algorithms to be supported. Read more <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites">here</a>.</td>
--- End diff --
ah you didn't escape any descriptions yet, nvm.
---