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
