Hi Andrey, Ah ok ... now I understand. I agree that I also like this approach ... it keeps the connection cleaner. And I guess such a Metadata object could not only contain such information about the capabilities, but also the concrete type of the PLC a connection is connected to, Versions etc. I could imagine that some supported functions are not only limited by the driver itself, but by the PLC model used. At least the supported datatypes is highly dependent on the type of S7 device. So I would definitely +1 to go down this Metadata path.
Chris Am 07.10.18, 19:46 schrieb "Andrey Skorikov" <[email protected]>: Hi Chris, I agree. As for now, the PR is already quite large and I would not like to let it grow further. A metadata object returned by some operation on PlcConnection (for example getMetadata() or getCapabilities()) would bundle all the operations for obtaining information about the connection itself. This is in contrast to the operational interface of the connection, which is used to actually perform the operations like reading/writing. Basically, all the canXYZ operations discussed so far can be bundled into one interface, and that would constitute the management interface (for obtaining metainformation) of the collection. As Julian pointed out, this pattern is employed in the java.sql API: https://docs.oracle.com/javase/7/docs/api/java/sql/DatabaseMetaData.html. The corresponding operation to obtain an instance of that type is https://docs.oracle.com/javase/7/docs/api/java/sql/Connection.html#getMetaData(). Another example is the JMX instrumentation level API for dynamic beans: https://docs.oracle.com/javase/7/docs/api/javax/management/DynamicMBean.html#getMBeanInfo(). However, I believe that at this stage there is no need to provide a separate interface for that, and having simple canRead()/canWrite() directly on PlcConnection would be sufficient. Andrey On 10/07/2018 06:17 PM, Christofer Dutz wrote: > Hi Julian, > > I agree that we should merge things asap ... just because something is merged, doesn't mean we can't fine-tune it after that. > I did have a look at the changes and I think it's safe to continue down that path. > > Also I like the idea of getting rid of the Optional ... it was annoying me too for quite some time. So having a "canXYZ" and a companion "getXYZRequestBuilder" methods sounds perfect from my side. > This way we can go the extra step of ensuring support, but can omit it where we just don't need it. > > Haven't quite understood the whole "Metadata" thing though ... ;-) > > Chris > > > Am 07.10.18, 15:15 schrieb "Julian Feinauer" <[email protected]>: > > Hey all, > > one more question. > Do we do the suggested changes in Andreys PR Branch or do we do it separately. > Then, we should try to merge this branch ASAP to have it there and to avoid merge hell (see https://media.giphy.com/media/cFkiFMDg3iFoI/giphy.gif). > > Personally, I feel unable to do a Code Review in the original sense (105 changes). > So after going through the API changes I definitely +1 them but I'm unsure if a "proper" Code Review is possible / necessary (so would agree on merging directly). > > What do others think? > > Julian > > Am 06.10.18, 21:20 schrieb "Julian Feinauer" <[email protected]>: > > Hey Andrey, > > I have to admit that your naming is definetly better than mine. > And I like your idea about this Metadata thing a lot. > I just checked how this is named in JDBC and the respective class is https://docs.oracle.com/javase/7/docs/api/java/sql/DatabaseMetaData.html > > So I think we can provide a canRead / canWrite (canSubscribe is a bit difficult, as we already hat several discussions about if we implement that by polling by default). > But I would also like the idea of having such a Metadata interface to transport further information about the PLC (like if this is native subscribing or polling and all such stuff). > > Best > Julian > > Am 06.10.18, 21:08 schrieb "Andrey Skorikov" <[email protected]>: > > Hello Julian, > > I think that a canRead()/canWrite()/canSubscribe() methods signaling > whether the connection supports reading/writing/subscription is a really > good solution. This would cleanly separate querying the meta-information > of a connection (whether the connection provides the required > capability) from actually using it, and would free the client from > dealing with the Optional<?>s all the time. > > There are also some alternative solutions: > > - Provide the meta-information in a separate data structure, returned by > some operation like getCapabilites() on PlcConnection. This can be > modeled in great detail or very simply (for example by returning a > BitSet). The client would check whether the required operation is > supported by calling operations on that object. > > - Provide "mix-in" interfaces, for example Readable and Writable. The > client would check whether the connection supports reading by evaluating > whether the connection object implements the required interface (for > example: connection instanceof Readable) and casting the connection to > that type. > > - Provide no meta-information at all and just throw an exception when a > unsupported operation is invoked. Would not recommend that, but still :-) > > In total, I think that Julian's solution (canRead() with Exception > thrown when a unsupported operation is invoked) balances the complexity > and flexibility best. > > Andrey > > > On 10/06/2018 08:38 PM, Julian Feinauer wrote: > > Hey everybody, > > > > I’m currently groing through Andreys PR (https://github.com/apache/incubator-plc4x/pull/27) which introduces some very good API changes and makes the API a lot more concise. > > But one thing that annoys me from the first day on plc4x is still there (and is now even more annoying as the rest is so clean). It is the boilerplate code I have write all the time when “just doing a connection to read something” due to the Optional<?> for getting the reader (or now the ReadRequestBuilder). > > For me, the getReader (or now readRequestBuilder) as Optional is like what Sebastian hates about Checked Exceptions. > > I never had to deal with a Connection which did not have a Reader but I had to check the Optional… at least 50 times, perhaps even more. > > > > Can’t we come up with a solution for that which would make the API (from my perspective) even more clean and user friendly. > > > > Suggestions could be: > > > > 1. Replace the connection directly with Reader, so no getConnection but getReader (or readRequestBuilder). And if this fails, it throws a PlcConnectionException, as usual. > > 2. No optional but another or canRead() method (for those who like it save) and it then throws a unchecked PlcConnectionException (or some subclass) > > > > What do the others think? Is this only me having the feeling that this is the same anti-pattern as with the checked exceptions? > > > > Julian > > > > > > >
