chrisdutz commented on code in PR #1468:
URL: https://github.com/apache/plc4x/pull/1468#discussion_r1538698841
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadRequest.java:
##########
@@ -38,6 +39,16 @@ interface Builder extends PlcRequestBuilder {
Builder addTagAddress(String name, String tagAddress);
Builder addTag(String name, PlcTag tag);
+ /**
+ * Parses a string representation of a tag address into a {@link
PlcTag} object.
+ * <p>
+ * This method safely attempts to convert the given tag address string
into a PlcTag, returning an
+ * {@link Optional} that is empty if the parsing fails.
+ *
+ * @param tagAddress The string representation of the tag address to
parse.
+ * @return An optional holding the parsed PLC tag if successful,
otherwise an empty optional.
+ */
+ Optional<PlcTag> parseSafe(String tagAddress);
Review Comment:
I would prefer "parseTagAddress" for this to match the naming pattern of the
other methods.
##########
plc4j/spi/src/main/java/org/apache/plc4x/java/spi/messages/DefaultPlcReadRequest.java:
##########
@@ -139,6 +136,19 @@ public PlcReadRequest build() {
return new DefaultPlcReadRequest(reader, parsedTags);
}
+ public PlcTag parseOrNull(String tagAddress){
Review Comment:
Generally the same comment as for the previous, in addition I personally
don't really like the "or-null" variant ... I would prefer to keep only the
optional one.
##########
plc4j/tools/connection-cache/src/main/java/org/apache/plc4x/java/utils/cache/LeasedPlcConnection.java:
##########
@@ -176,6 +176,11 @@ public PlcReadRequest.Builder addTagAddress(String name,
String tagAddress) {
public PlcReadRequest.Builder addTag(String name, PlcTag tag) {
return innerBuilder.addTag(name, tag);
}
+
+ @Override
+ public Optional<PlcTag> parseSafe(String tagAddress) {
Review Comment:
Same comment as, I think this would be a better fit in the connection
itself, not in the builder.
##########
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadRequest.java:
##########
@@ -38,6 +39,16 @@ interface Builder extends PlcRequestBuilder {
Builder addTagAddress(String name, String tagAddress);
Builder addTag(String name, PlcTag tag);
+ /**
+ * Parses a string representation of a tag address into a {@link
PlcTag} object.
+ * <p>
+ * This method safely attempts to convert the given tag address string
into a PlcTag, returning an
+ * {@link Optional} that is empty if the parsing fails.
+ *
+ * @param tagAddress The string representation of the tag address to
parse.
+ * @return An optional holding the parsed PLC tag if successful,
otherwise an empty optional.
+ */
+ Optional<PlcTag> parseSafe(String tagAddress);
Review Comment:
Also not 100% sure, if we should not instead have this method in the
connection instead ... reasoning is that when adding it to the builder it
doesn't really add anything to the request it builds .... however having a
"parseTagAddress" and "parseQueryAddress" in the connection sounds like a very
cool thing to do, as this way tooling utilizing PLC4X can use to validate tags
and queries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@plc4x.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
