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
