[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833353
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/specific/TypeSafePlcReadResponse.java
 ##
 @@ -18,13 +18,13 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.api.messages.specific;
 
-import org.apache.plc4x.java.api.messages.PlcReadResponse;
-import org.apache.plc4x.java.api.messages.items.ReadResponseItem;
-
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 
+import org.apache.plc4x.java.api.messages.PlcReadResponse;
+import org.apache.plc4x.java.api.messages.items.ReadResponseItem;
+
 
 Review comment:
   import reordering see other comment


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833183
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/specific/TypeSafePlcWriteRequest.java
 ##
 @@ -71,7 +71,7 @@ public void addItem(WriteRequestItem writeRequestItem) {
 
 @SuppressWarnings("unchecked")
 public List getCheckedRequestItems() {
-return (List) getRequestItems();
+return (List) ((Object) getRequestItems());
 
 Review comment:
   strange cast see other comment


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170831995
 
 

 ##
 File path: 
examples/dummy-driver/src/main/java/org/apache/plc4x/java/examples/dummydriver/netty/DummyProtocol.java
 ##
 @@ -18,26 +18,27 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.examples.dummydriver.netty;
 
-import io.netty.buffer.ByteBuf;
-import io.netty.buffer.ByteBufUtil;
-import io.netty.buffer.Unpooled;
-import io.netty.channel.ChannelHandlerContext;
-import io.netty.handler.codec.MessageToMessageCodec;
+import java.util.List;
+
 import org.apache.plc4x.java.api.messages.PlcReadRequest;
 import org.apache.plc4x.java.api.messages.PlcRequest;
 import org.apache.plc4x.java.api.messages.PlcRequestContainer;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.List;
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.ByteBufUtil;
+import io.netty.buffer.Unpooled;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.handler.codec.MessageToMessageCodec;
 
 Review comment:
   we should look into why eclipse does sort the imports other than idea. We 
have a [.editorconfig](http://editorconfig.org/) (which eclipse doesn't do 
out-of-box and needs the plugin installed) but I don't think it respects import 
order. Without a unified import order we mess up our diff from time to time.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833285
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadResponse.java
 ##
 @@ -18,13 +18,13 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.api.messages;
 
-import org.apache.plc4x.java.api.messages.items.ReadRequestItem;
-import org.apache.plc4x.java.api.messages.items.ReadResponseItem;
-
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 
+import org.apache.plc4x.java.api.messages.items.ReadRequestItem;
+import org.apache.plc4x.java.api.messages.items.ReadResponseItem;
+
 
 Review comment:
   import reordering see other comment


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833513
 
 

 ##
 File path: 
plc4j/core/src/test/java/org/apache/plc4x/java/PlcDriverManagerTest.java
 ##
 @@ -26,21 +37,11 @@ Licensed to the Apache Software Foundation (ASF) under one
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
-import java.io.File;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLClassLoader;
-
-import static org.hamcrest.core.Is.is;
-import static org.hamcrest.core.IsInstanceOf.instanceOf;
-import static org.hamcrest.core.IsNull.notNullValue;
-import static org.hamcrest.core.IsNull.nullValue;
-import static org.junit.Assert.assertThat;
-
 public class PlcDriverManagerTest {
 
 /**
  * Tries to get the mock plc driver which is part of this testsuite.
+ * @throws PlcException 
 
 Review comment:
   theses doesn't make sense in Unit-Tests. Better just remove on star `*`


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170832428
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/specific/TypeSafePlcReadResponse.java
 ##
 @@ -59,17 +59,49 @@ public TypeSafePlcReadResponse(TypeSafePlcReadRequest 
request, List>) super.getResponseItem();
 }
 
+/**
+ * Cast or convert a PlcReadResponse to a TypeSafePlcReadReadResponse.
+ * 
+ * WARNING: this is inherently a non-type-safe operation.  It was 
introduced
+ * to serve the implementation of 
PlcReader.read(TypeSafePlcReadRequest).
+ * Additional use of it is not recommended.  This interface is subject to 
change.
+ * 
+ * @param plcReadResponse the response implicitly with items of type T
+ * @return TypeSafePlcReadReadResponse
+ */
 @SuppressWarnings("unchecked")
 public static  TypeSafePlcReadResponse of(PlcReadResponse 
plcReadResponse) {
 
 Review comment:
   fair Point, we would need a type Parameter `Class clazz` here.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833397
 
 

 ##
 File path: 
plc4j/core/src/test/java/org/apache/plc4x/java/PlcDriverManagerTest.java
 ##
 @@ -26,21 +37,11 @@ Licensed to the Apache Software Foundation (ASF) under one
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 
-import java.io.File;
-import java.net.MalformedURLException;
-import java.net.URL;
-import java.net.URLClassLoader;
-
-import static org.hamcrest.core.Is.is;
-import static org.hamcrest.core.IsInstanceOf.instanceOf;
-import static org.hamcrest.core.IsNull.notNullValue;
-import static org.hamcrest.core.IsNull.nullValue;
-import static org.junit.Assert.assertThat;
-
 
 Review comment:
   import reordering see other comment


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833373
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/specific/TypeSafePlcWriteRequest.java
 ##
 @@ -18,14 +18,14 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.api.messages.specific;
 
-import org.apache.plc4x.java.api.messages.PlcWriteRequest;
-import org.apache.plc4x.java.api.messages.items.WriteRequestItem;
-import org.apache.plc4x.java.api.model.Address;
-
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 
+import org.apache.plc4x.java.api.messages.PlcWriteRequest;
+import org.apache.plc4x.java.api.messages.items.WriteRequestItem;
+import org.apache.plc4x.java.api.model.Address;
+
 
 Review comment:
   import reordering see other comment


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833305
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcWriteRequest.java
 ##
 @@ -18,13 +18,13 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.api.messages;
 
+import java.util.List;
+import java.util.Objects;
+
 import org.apache.plc4x.java.api.messages.items.WriteRequestItem;
 import org.apache.plc4x.java.api.messages.specific.TypeSafePlcWriteRequest;
 import org.apache.plc4x.java.api.model.Address;
 
-import java.util.List;
-import java.util.Objects;
-
 
 Review comment:
   import reordering see other comment


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833118
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/specific/TypeSafePlcReadResponse.java
 ##
 @@ -84,7 +116,9 @@ public TypeSafePlcReadResponse(TypeSafePlcReadRequest 
request, List

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170832166
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcWriteResponse.java
 ##
 @@ -37,6 +37,7 @@ public PlcWriteResponse(PlcWriteRequest request, List Optional getValue(WriteRequestItem 
item) {
-return (Optional) super.getValue(item);
+Optional value = super.getValue(item); 
+return (Optional) ((Object) value); 
 
 Review comment:
   This cast to Object and then a type again is a bit confusing. Is this just 
to silence a warning?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170833246
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadRequest.java
 ##
 @@ -18,13 +18,13 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.api.messages;
 
+import java.util.List;
+import java.util.Objects;
+
 import org.apache.plc4x.java.api.messages.items.ReadRequestItem;
 import org.apache.plc4x.java.api.messages.specific.TypeSafePlcReadRequest;
 import org.apache.plc4x.java.api.model.Address;
 
-import java.util.List;
-import java.util.Objects;
-
 
 Review comment:
   import reordering see other comment


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, plc4j-core, dummy-driver

[GitBox]

sruehl commented on a change in pull request #8: cleanup warnings in plc4j-api, 
plc4j-core, dummy-driver
URL: https://github.com/apache/incubator-plc4x/pull/8#discussion_r170832548
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/specific/TypeSafePlcReadResponse.java
 ##
 @@ -59,17 +59,49 @@ public TypeSafePlcReadResponse(TypeSafePlcReadRequest 
request, List>) super.getResponseItem();
 }
 
+/**
+ * Cast or convert a PlcReadResponse to a TypeSafePlcReadReadResponse.
+ * 
+ * WARNING: this is inherently a non-type-safe operation.  It was 
introduced
+ * to serve the implementation of 
PlcReader.read(TypeSafePlcReadRequest).
+ * Additional use of it is not recommended.  This interface is subject to 
change.
+ * 
+ * @param plcReadResponse the response implicitly with items of type T
+ * @return TypeSafePlcReadReadResponse
+ */
 @SuppressWarnings("unchecked")
 public static  TypeSafePlcReadResponse of(PlcReadResponse 
plcReadResponse) {
+// BUG: there seems to be no checking that the readResponse/items
+// in fact are for type T.
+// I don't even think it's possible to do that with the current 'of()' 
signature
+// and plcReadResponse content.
+//
+// The only consolation is that currently, 'of()' is only really used 
in the
+// impl of PlcReader.read(TypeSafePlcReadRequest) and that case 
guarantees
+// that all items are a T.  plcReadResponse isa 
TypeSafePlcReadResponse in
+// this case.
+//
+// Maybe we just need to doc that this conversion is unsafe and/or 
rename
+// the method to "unsafeOf()"? 
+// Or, if there were an AbstractPlcReader, that could internally 
implement
+// this method without exposing this generally it, the PlcReader 
interface
+// could remove the default implementation of 
read(TypeSafePlcReadRequest),
+// and protocol implementations could extend AbstractPlcReader.
+//
+// FWIW, in one case there is some checking that all of the items in a 
response
+// are at least of the same type.
+  
 if (plcReadResponse instanceof TypeSafePlcReadResponse) {
-return (TypeSafePlcReadResponse) plcReadResponse;
+// Warning: no validation that everything in the response is a T.
 
 Review comment:
   then we would need to check here if the requested `Class class` is equals to 
the supplied type


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] asfgit closed pull request #7: cleanup some warnings (in Eclipse)

[GitBox]

asfgit closed pull request #7: cleanup some warnings (in Eclipse)
URL: https://github.com/apache/incubator-plc4x/pull/7
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/examples/dummy-driver/src/main/java/org/apache/plc4x/java/examples/dummydriver/connection/DummyConnection.java
 
b/examples/dummy-driver/src/main/java/org/apache/plc4x/java/examples/dummydriver/connection/DummyConnection.java
index 9619fe4c..a61686bc 100644
--- 
a/examples/dummy-driver/src/main/java/org/apache/plc4x/java/examples/dummydriver/connection/DummyConnection.java
+++ 
b/examples/dummy-driver/src/main/java/org/apache/plc4x/java/examples/dummydriver/connection/DummyConnection.java
@@ -18,13 +18,18 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.examples.dummydriver.connection;
 
-import io.netty.bootstrap.Bootstrap;
-import io.netty.channel.*;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
 import org.apache.plc4x.java.api.connection.AbstractPlcConnection;
 import org.apache.plc4x.java.api.connection.PlcReader;
 import org.apache.plc4x.java.api.connection.PlcWriter;
 import org.apache.plc4x.java.api.exceptions.PlcConnectionException;
-import org.apache.plc4x.java.api.messages.*;
+import org.apache.plc4x.java.api.messages.PlcReadRequest;
+import org.apache.plc4x.java.api.messages.PlcReadResponse;
+import org.apache.plc4x.java.api.messages.PlcRequestContainer;
+import org.apache.plc4x.java.api.messages.PlcWriteRequest;
+import org.apache.plc4x.java.api.messages.PlcWriteResponse;
 import org.apache.plc4x.java.api.model.Address;
 import org.apache.plc4x.java.examples.dummydriver.model.DummyAddress;
 import org.apache.plc4x.java.examples.dummydriver.netty.DummyProtocol;
@@ -33,17 +38,24 @@ Licensed to the Apache Software Foundation (ASF) under one
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.ExecutionException;
+import io.netty.bootstrap.Bootstrap;
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelInitializer;
+import io.netty.channel.ChannelPipeline;
+import io.netty.channel.DefaultEventLoopGroup;
+import io.netty.channel.EventLoopGroup;
 
 public class DummyConnection extends AbstractPlcConnection implements 
PlcReader, PlcWriter {
 
+@SuppressWarnings("unused")
 private static final Logger logger = 
LoggerFactory.getLogger(DummyConnection.class);
 
 private final String hostName;
 
 private EventLoopGroup workerGroup;
 private Channel channel;
+@SuppressWarnings("unused")
 private boolean connected;
 
 public DummyConnection(String hostName) {
@@ -71,7 +83,7 @@ public void connect() throws PlcConnectionException {
 Bootstrap bootstrap = new Bootstrap();
 bootstrap.group(workerGroup);
 bootstrap.channel(RawSocketChannel.class);
-bootstrap.handler(new ChannelInitializer() {
+bootstrap.handler(new ChannelInitializer() {
 @Override
 protected void initChannel(Channel channel) throws Exception {
 ChannelPipeline pipeline = channel.pipeline();
diff --git 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcRequest.java 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcRequest.java
index da2fed4a..d41453af 100644
--- a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcRequest.java
+++ b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcRequest.java
@@ -18,15 +18,16 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.api.messages;
 
-import org.apache.plc4x.java.api.messages.items.RequestItem;
-
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
 
+import org.apache.plc4x.java.api.messages.items.RequestItem;
+
 /**
  * Base type for all messages sent from the plc4x system to a connected plc.
+ * @param  
  */
 public abstract class PlcRequest implements 
PlcMessage {
 
diff --git 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcResponse.java 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcResponse.java
index 1ab12820..8f054cc4 100644
--- 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcResponse.java
+++ 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcResponse.java
@@ -18,16 +18,19 @@ Licensed to the Apache Software Foundation (ASF) under one
 */
 package org.apache.plc4x.java.api.messages;
 
-import org.apache.plc4x.java.api.messages.items.RequestItem;
-import 

Re: 235 warnings

[Justin Mclean]

Hi,

> I'd be inclined to do a bunch of the cleanup myself if I had some degree of 
> confidence that the project was committed to their elimination.  A project 
> policy of not delivering code that adds more is sufficient IMO (not sure 
> tooling enforcement is necessary). Can IntelliJ be configured to always show 
> to the warnings so folks can avoid them?


Is eclipse using FindBugs or something similar? There may be a similar plugin 
for intellij. IntelliJ can run sonarqube (via sonarlint) on the code in the IDE 
and also does t own analysis.

IMO we shovel try and fix where possible and it can as you pointed out hide 
more serious issues.

Thanks,
Justin

Re: 235 warnings

[Justin Mclean]

Hi,

> Sonar[1] reports 158 code smells but those don’t seem to include any of the 
> categories of the 235 noted below.  Not sure what to make of that.

It could be that some rules have been turned off? I’m also seeing some sonar 
cube one that are legitimate issues but nothing too serious. Some of the issues 
are due to code that is not complete yet. And a couple of others it probably 
best not to fix until we have code coverage by tests in that area.

> At least an overwhelming number of the 235 are legitimate.  E.g., one should 
> use a generic type in a generic way instead of as a raw type; there are in 
> fact unnecessary @SuppressWarning instances and missing javadoc tags, etc.

If you think they are legitimate please raise an issue and/or go ahead and fix 
them.

Thanks,
Justin

[GitHub] asfgit closed pull request #6: cleanup sonar edgent complaints

[GitBox]

asfgit closed pull request #6: cleanup sonar edgent complaints
URL: https://github.com/apache/incubator-plc4x/pull/6
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/integrations/apache-edgent/src/main/java/org/apache/plc4x/edgent/PlcConnectionAdapter.java
 
b/integrations/apache-edgent/src/main/java/org/apache/plc4x/edgent/PlcConnectionAdapter.java
index eedfc794..bc4eaefe 100644
--- 
a/integrations/apache-edgent/src/main/java/org/apache/plc4x/edgent/PlcConnectionAdapter.java
+++ 
b/integrations/apache-edgent/src/main/java/org/apache/plc4x/edgent/PlcConnectionAdapter.java
@@ -108,28 +108,38 @@ public Address parseAddress(String addressString) throws 
PlcException {
 
  Supplier newSupplier(Class datatype, String addressStr) {
 PlcConnectionAdapter.checkDatatype(datatype);
-return new Supplier() {
-private static final long serialVersionUID = 1L;
+// satisfy sonar's "Reduce number of anonymous class lines" code smell
+return new MySupplier(datatype, addressStr);
+}
+
+private class MySupplier implements Supplier {
+private static final long serialVersionUID = 1L;
+private Class datatype;
+private String addressStr;
+  
+MySupplier(Class datatype, String addressStr) {
+this.datatype = datatype;
+this.addressStr = addressStr;
+}
 
-@Override
-public T get() {
-PlcConnection connection = null;
-Address address = null;
-try {
-connection = getConnection();
-address = connection.parseAddress(addressStr);
-PlcReader reader = connection.getReader()
-.orElseThrow(() -> new NullPointerException("No reader 
available"));
-TypeSafePlcReadRequest readRequest = 
PlcConnectionAdapter.newPlcReadRequest(datatype, address);
-return reader.read(readRequest).get().getResponseItem()
-.orElseThrow(() -> new IllegalStateException("No 
response available"))
-.getValues().get(0);
-} catch (Exception e) {
-logger.error("reading from plc device {} {} failed", 
connection, address, e);
-return null;
-}
+@Override
+public T get() {
+PlcConnection connection = null;
+Address address = null;
+try {
+connection = getConnection();
+address = connection.parseAddress(addressStr);
+PlcReader reader = connection.getReader()
+  .orElseThrow(() -> new NullPointerException("No reader 
available"));
+TypeSafePlcReadRequest readRequest = 
PlcConnectionAdapter.newPlcReadRequest(datatype, address);
+return reader.read(readRequest).get().getResponseItem()
+  .orElseThrow(() -> new IllegalStateException("No response 
available"))
+  .getValues().get(0);
+} catch (Exception e) {
+logger.error("reading from plc device {} {} failed", 
connection, address, e);
+return null;
 }
-};
+}
 }
 
 Supplier newSupplier(PlcReadRequest readRequest) {


 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] dlaboss opened a new pull request #6: cleanup sonar edgent complaints

[GitBox]

dlaboss opened a new pull request #6: cleanup sonar edgent complaints
URL: https://github.com/apache/incubator-plc4x/pull/6
 
 
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Re: 235 warnings

[Dale LaBossiere]

Sonar[1] reports 158 code smells but those don’t seem to include any of the 
categories of the 235 noted below.  Not sure what to make of that.

At least an overwhelming number of the 235 are legitimate.  E.g., one should 
use a generic type in a generic way instead of as a raw type; there are in fact 
unnecessary @SuppressWarning instances and missing javadoc tags, etc.

[1] 
https://builds.apache.org/analysis/dashboard/index/org.apache.plc4x:plc4x-parent

> On Feb 26, 2018, at 11:13 AM, Christofer Dutz  
> wrote:
> 
> Hi Dale,
> 
> I'm seeing similar Problems as with Edgent here ;-)
> 
> If the reported problems are actual problems, we should fix them and find 
> ways to prevent them in the future by making IntelliJ report them too. 
> However my trust in reports of Eclipe is not too high. Is Sonar reporting 
> them too? 
> 
> Chris
> 
> 
> 
> 
> Am 26.02.18, 15:56 schrieb "Dale LaBossiere" :
> 
>There were many in test code but lots in non-test code too.  I don’t think 
> there are serious problems.  But with all of the warnings it’s too easy to 
> miss significant ones or add more.
> 
>Changing “Use of a raw type” from warning to info drops the warnings from 
> 235 to 85.
>Then changing “Unchecked generic type operation” to info drops the 
> warnings to 47.
>That left some javadoc, pom (duplicate version), unnecessary and 
> unsupported @SuppressWarnings, and unused variables.
> 
>I'd be inclined to do a bunch of the cleanup myself if I had some degree 
> of confidence that the project was committed to their elimination.  A project 
> policy of not delivering code that adds more is sufficient IMO (not sure 
> tooling enforcement is necessary). Can IntelliJ be configured to always show 
> to the warnings so folks can avoid them?
> 
>How does the rest of the team feel about all of this?
> 
>— Dale
> 
> 
>> On Feb 24, 2018, at 7:02 PM, Justin Mclean  wrote:
>> 
>> Hi,
>> 
>>> On the current “master” branch, Eclipse 4.6.3 (with std Error/Warning 
>>> preference config) and java 1.8.0_161, Eclipse reports 235 warnings. The 
>>> bulk of these seem to be Raw type and Type safety warnings.  Are there 
>>> plans for dealing with these?
>> 
>> I guess you are project the only person using Eclipse. IntelliJ also gives 
>> number of warnings when you analyse the code, but most of the “issues” are 
>> with test code and not issues or due to a couple of things that have not 
>> been completed. Sonarqube also does a good job of picking most things. Are 
>> any of the issues Eclipse picks up in your opinion serious? 
>> 
>> Thanks,
>> Justin
> 
> 
> 

Re: 235 warnings

[Dale LaBossiere]

There were many in test code but lots in non-test code too.  I don’t think 
there are serious problems.  But with all of the warnings it’s too easy to miss 
significant ones or add more.

Changing “Use of a raw type” from warning to info drops the warnings from 235 
to 85.
Then changing “Unchecked generic type operation” to info drops the warnings to 
47.
That left some javadoc, pom (duplicate version), unnecessary and unsupported 
@SuppressWarnings, and unused variables.

I'd be inclined to do a bunch of the cleanup myself if I had some degree of 
confidence that the project was committed to their elimination.  A project 
policy of not delivering code that adds more is sufficient IMO (not sure 
tooling enforcement is necessary). Can IntelliJ be configured to always show to 
the warnings so folks can avoid them?

How does the rest of the team feel about all of this?

— Dale


> On Feb 24, 2018, at 7:02 PM, Justin Mclean  wrote:
> 
> Hi,
> 
>> On the current “master” branch, Eclipse 4.6.3 (with std Error/Warning 
>> preference config) and java 1.8.0_161, Eclipse reports 235 warnings. The 
>> bulk of these seem to be Raw type and Type safety warnings.  Are there plans 
>> for dealing with these?
> 
> I guess you are project the only person using Eclipse. IntelliJ also gives 
> number of warnings when you analyse the code, but most of the “issues” are 
> with test code and not issues or due to a couple of things that have not been 
> completed. Sonarqube also does a good job of picking most things. Are any of 
> the issues Eclipse picks up in your opinion serious? 
> 
> Thanks,
> Justin