You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by Christofer Dutz <ch...@c-ware.de> on 2018/01/04 15:08:09 UTC
Generify branch
Hi all,
as I promised to Sebastian, I just checked the changes in the generification branch.
One thing I did notice, was that one ReadRequest can have different types of items inside … should the ReadRequest / WriteRequest be fixed to one generic type in both cases?
Something like this:
PlcReadRequest req = new PlcReadRequest();
req.addItem(new ReadRequestItem(Byte.class, inputs));
req.addItem(new ReadRequestItem(Boolean.class, outputs, 3));
CompletableFuture future = plcReader.read(req);
PlcReadResponse resp = (PlcReadResponse) future.get();
It does produce warnings, the way it’s currently implemented.
Right now, my version allows using all types of items inside one request (even mixed), but we lost generification. Eventually it would be good to have a Read/WriteRequest and a Batch version that’s not generic.
What do you think? Right now, I would opt to not merge the changes back until we have discussed this fully.
Chris
Re: Generify branch
Posted by Mark Keinhörster <ma...@codecentric.de>.
Goode evening,
I removed the reference and pushed to master.
The current changes got also merged into the PLC4X-12 branch and is in working state.
Enjoy the evening,
Mark
> Am 12.01.2018 um 18:04 schrieb Christofer Dutz <ch...@c-ware.de>:
>
> +1 for not having a reference from the request to the response.
>
> Chris
>
> Outlook for Android<https://aka.ms/ghei36> herunterladen
>
>
>
> Von: Mark Keinhörster
> Gesendet: Freitag, 12. Januar, 17:27
> Betreff: Re: Generify branch
> An: dev@plc4x.apache.org
>
>
> Hi Sebastian, cool that looks a lot cleaner than before, thank you so much! :) As I try to merge the changes into the Scala branch, there is one last point to discuss: Do we really need the PlcRequest reference the response? I can relate to it in terms of convenience but it looks quite ugly to me for the following reasons: 1) The two classes (Response/Request) are really tightly coupled which is not necessary as we can „navigate“ back from response to request 2) We got mutable state in the request item which is not functional and I would hesitate to implement it that way in Scala 3) The behavior gets quite messy when a RequestItem instance is used multiple times (which right now is viable and not prohibited) Just one example: Sending an item a second time leads to a non-updated request item as the response item won’t be null anymore. Experience has shown me that constructs like this lead to really hard-to-find bugs, but I am open for discussion. @Chris what do you think? Sorry for nagging again... again a big thanks for the clean-up! Mark > Am 11.01.2018 um 15:35 schrieb Sebastian Rühl : > > Hi all, > > I could cleanup a lot in the refactoring/java_generify branch. > > First of all thanks for the feedback of Chris and Mark I could simplify this by a great deal. > Now we just have additional TypeSafePlc* requests. > The convenience methods are integrated into the normal request classes. > > As the changes are now minimal im going to merge this branch into master. > > Regarding the builders: My preference would be to encourage the use of them and limit the visibility of the constructors. However as these are optimizations we could look at this after that. > > Sebastian
>
Re: Generify branch
Posted by Christofer Dutz <ch...@c-ware.de>.
+1 for not having a reference from the request to the response.
Chris
Outlook for Android<https://aka.ms/ghei36> herunterladen
Von: Mark Keinhörster
Gesendet: Freitag, 12. Januar, 17:27
Betreff: Re: Generify branch
An: dev@plc4x.apache.org
Hi Sebastian, cool that looks a lot cleaner than before, thank you so much! :) As I try to merge the changes into the Scala branch, there is one last point to discuss: Do we really need the PlcRequest reference the response? I can relate to it in terms of convenience but it looks quite ugly to me for the following reasons: 1) The two classes (Response/Request) are really tightly coupled which is not necessary as we can „navigate“ back from response to request 2) We got mutable state in the request item which is not functional and I would hesitate to implement it that way in Scala 3) The behavior gets quite messy when a RequestItem instance is used multiple times (which right now is viable and not prohibited) Just one example: Sending an item a second time leads to a non-updated request item as the response item won’t be null anymore. Experience has shown me that constructs like this lead to really hard-to-find bugs, but I am open for discussion. @Chris what do you think? Sorry for nagging again... again a big thanks for the clean-up! Mark > Am 11.01.2018 um 15:35 schrieb Sebastian Rühl : > > Hi all, > > I could cleanup a lot in the refactoring/java_generify branch. > > First of all thanks for the feedback of Chris and Mark I could simplify this by a great deal. > Now we just have additional TypeSafePlc* requests. > The convenience methods are integrated into the normal request classes. > > As the changes are now minimal im going to merge this branch into master. > > Regarding the builders: My preference would be to encourage the use of them and limit the visibility of the constructors. However as these are optimizations we could look at this after that. > > Sebastian
Re: Generify branch
Posted by Mark Keinhörster <ma...@codecentric.de>.
Hi Sebastian,
cool that looks a lot cleaner than before, thank you so much! :)
As I try to merge the changes into the Scala branch, there is one last point to discuss:
Do we really need the PlcRequest reference the response? I can relate to it in terms of convenience
but it looks quite ugly to me for the following reasons:
1) The two classes (Response/Request) are really tightly coupled which is not necessary as we can „navigate“ back from response to request
2) We got mutable state in the request item which is not functional and I would hesitate to implement it that way in Scala
3) The behavior gets quite messy when a RequestItem instance is used multiple times (which right now is viable and not prohibited)
Just one example: Sending an item a second time leads to a non-updated request item as the response item won’t be null anymore.
Experience has shown me that constructs like this lead to really hard-to-find bugs, but I am open for discussion.
@Chris what do you think?
Sorry for nagging again...
again a big thanks for the clean-up!
Mark
> Am 11.01.2018 um 15:35 schrieb Sebastian Rühl <se...@googlemail.com>:
>
> Hi all,
>
> I could cleanup a lot in the refactoring/java_generify branch.
>
> First of all thanks for the feedback of Chris and Mark I could simplify this by a great deal.
> Now we just have additional TypeSafePlc* requests.
> The convenience methods are integrated into the normal request classes.
>
> As the changes are now minimal im going to merge this branch into master.
>
> Regarding the builders: My preference would be to encourage the use of them and limit the visibility of the constructors. However as these are optimizations we could look at this after that.
>
> Sebastian
Re: Generify branch
Posted by Sebastian Rühl <se...@googlemail.com>.
Hi all,
I could cleanup a lot in the refactoring/java_generify branch.
First of all thanks for the feedback of Chris and Mark I could simplify this by a great deal.
Now we just have additional TypeSafePlc* requests.
The convenience methods are integrated into the normal request classes.
As the changes are now minimal im going to merge this branch into master.
Regarding the builders: My preference would be to encourage the use of them and limit the visibility of the constructors. However as these are optimizations we could look at this after that.
Sebastian
Re: Generify branch
Posted by Sebastian Rühl <se...@googlemail.com>.
Hi all,
Regarding the different request response types, I might just strip it down again (I tag it again before so we can use this as reference/branchpoint).
I quite like the getValue() one problem though is that you always need to iterate a list but I guess one could live with that so great addition.
Additionally I added a simple `getResponseItem()` to the RequestItem which returns an optional so we can use these references to get a typed response.
Sebastian
Re: Generify branch
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi all,
had to double check and it took quite a while till I noticed that you were talking about the generify branch ;-)
I thought this had already been merged and just noticed it hadn’t. Perhaps it’s good this way as it seems we should probably discuss things in the team first.
Thinking more about the topic, I don’t think single-value and batch should be the discriminator on this topic, but single-type and multi-type. For single-type we could use generics to make casting obsolete, but is that worth the more complicated API? Right now (but that’s just my opinion) I would prefer the simple (non-generic) Read/Write Request/Response but to have generic Items. These generic Read/Write items could be used in conjunction with a generic getValue in the response class that returns the corresponding generic type of ReadResponseItem (in case of the write response, the response doesn’t require a generic type).
I do agree that the API should be as lightweight as possible. That was the reason for me removing all generic types 1 week before Christmas. But then I had not only generic types, but I also had to provide 2-3 classes per supported type. Getting rid of all of these classes was definitely a good choice. While I was at it, thinking mainly about optimized read operations (I guess most use-cases will be reading multiple values in one request), I thought the added complexity of having generic and non-generic classes/methods did not overweigh the increased complexity, so I removed the generic support completely.
Regarding the Builder/Direct Instantiation … what would be the benefit of providing a builder and preventing direct instantiation? I think, giving the user a choice is always something good. There are people that prefer builders and some don’t. We should support both (I think).
Perhaps it would have saved us a lot of time, if we had managed to really meet and do a proper API kick-off.
Chris
Am 07.01.18, 23:52 schrieb "Mark Keinhörster" <ma...@codecentric.de>:
Good evening,
short update on the generify:
-> I added the getValue-method as proposed to the bulk requests in the generify branch.
-> added unittests
-> noticed that the old unittests were not working and fixed them (that was quite unfortunate)
-> found a bug in the builder at PlcReadRequest, the initial check in „checkType“
should look if the type is null instead of != otherwise it will never be set.
private void checkType(Class dataType) {
if (firstType == null) {
firstType = dataType;
}
if (firstType != dataType) {
mixed = true;
}
}
—> changed one unittest to actually using the builder but this needs more testing if we stick to this
Feedback:
Overall it feels quite heavy with all these options:
—> Builders and direct instantiations
—> checked, unchecked, bulk, single requests/responses
To the first point I personally would not favor a any of the two but either stick with one or the other and not both.
To the second point, we can discuss what’s more suitable. I would prefer the unchecked version with „getValue“
but I am open to any argument/recommendation against it.
What do you think?
Enjoy the evening,
Mark
> Am 05.01.2018 um 13:43 schrieb Sebastian Rühl <se...@googlemail.com>:
>
> Hi Together,
>
> I refactored the branch (refactoring/java_generify) a bit so that you now can do following:
>
> PlcReadRequest req = new BulkPlcReadRequest()
> req.addItem(new ReadRequestItem<>(Byte.class, inputs));
> req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
> CompletableFuture future = plcReader.read(req);
> PlcReadResponse resp = (PlcReadResponse) future.get();
> Only difference on the requirement from Chris is, that you now need a specific request implementation and add the <> onto the Request Items.
> Currently following Types are implemented (PlcReadRequest* is a interface now):
> BulkPlc(Read|Write)Re(quest|ponse)
> CheckedBulkPlc(Read|Write)Re(quest|ponse)
> SinglePlc(Read|Write)Re(quest|ponse)
>
> The tag of the pre-refacotring is „pre_bulk_refactoring“ if needed for comparison (On branch refactoring/java_generify).
>
> Essential is the hierarchy of the implementation now:
>
> PlcReadRequest
> / \
> SinglePlcReadRequest BulkPlcReadRequest
> |
> CheckedBulkPlcReadRequest
>
> With this following Signatures can be used to conveniently work with strong typing:
>
> public interface PlcReader {
>
> CompletableFuture<? extends PlcReadResponse> read(PlcReadRequest readRequest);
>
> @SuppressWarnings("unchecked")
> default <T> CompletableFuture<SinglePlcReadResponse<T>> read(SinglePlcReadRequest<T> readRequest) {
> return (CompletableFuture<SinglePlcReadResponse<T>>) read((PlcReadRequest) readRequest);
> }
>
> @SuppressWarnings("unchecked")
> default CompletableFuture<BulkPlcReadResponse> read(BulkPlcReadRequest readRequest) {
> return (CompletableFuture<BulkPlcReadResponse>) read((PlcReadRequest) readRequest);
> }
>
> @SuppressWarnings("unchecked")
> default <T> CompletableFuture<CheckedBulkPlcReadResponse<T>> read(CheckedBulkPlcReadRequest<T> readRequest) {
> return (CompletableFuture<CheckedBulkPlcReadResponse<T>>) read((PlcReadRequest) readRequest);
> }
>
> }
>
>
> The mentioned hierarchy is essential to make the overloaded methods work.
>
> As always Im happy about feedback :)
>
> Sebastian
Re: Generify branch
Posted by Mark Keinhörster <ma...@codecentric.de>.
Good evening,
short update on the generify:
-> I added the getValue-method as proposed to the bulk requests in the generify branch.
-> added unittests
-> noticed that the old unittests were not working and fixed them (that was quite unfortunate)
-> found a bug in the builder at PlcReadRequest, the initial check in „checkType“
should look if the type is null instead of != otherwise it will never be set.
private void checkType(Class dataType) {
if (firstType == null) {
firstType = dataType;
}
if (firstType != dataType) {
mixed = true;
}
}
—> changed one unittest to actually using the builder but this needs more testing if we stick to this
Feedback:
Overall it feels quite heavy with all these options:
—> Builders and direct instantiations
—> checked, unchecked, bulk, single requests/responses
To the first point I personally would not favor a any of the two but either stick with one or the other and not both.
To the second point, we can discuss what’s more suitable. I would prefer the unchecked version with „getValue“
but I am open to any argument/recommendation against it.
What do you think?
Enjoy the evening,
Mark
> Am 05.01.2018 um 13:43 schrieb Sebastian Rühl <se...@googlemail.com>:
>
> Hi Together,
>
> I refactored the branch (refactoring/java_generify) a bit so that you now can do following:
>
> PlcReadRequest req = new BulkPlcReadRequest()
> req.addItem(new ReadRequestItem<>(Byte.class, inputs));
> req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
> CompletableFuture future = plcReader.read(req);
> PlcReadResponse resp = (PlcReadResponse) future.get();
> Only difference on the requirement from Chris is, that you now need a specific request implementation and add the <> onto the Request Items.
> Currently following Types are implemented (PlcReadRequest* is a interface now):
> BulkPlc(Read|Write)Re(quest|ponse)
> CheckedBulkPlc(Read|Write)Re(quest|ponse)
> SinglePlc(Read|Write)Re(quest|ponse)
>
> The tag of the pre-refacotring is „pre_bulk_refactoring“ if needed for comparison (On branch refactoring/java_generify).
>
> Essential is the hierarchy of the implementation now:
>
> PlcReadRequest
> / \
> SinglePlcReadRequest BulkPlcReadRequest
> |
> CheckedBulkPlcReadRequest
>
> With this following Signatures can be used to conveniently work with strong typing:
>
> public interface PlcReader {
>
> CompletableFuture<? extends PlcReadResponse> read(PlcReadRequest readRequest);
>
> @SuppressWarnings("unchecked")
> default <T> CompletableFuture<SinglePlcReadResponse<T>> read(SinglePlcReadRequest<T> readRequest) {
> return (CompletableFuture<SinglePlcReadResponse<T>>) read((PlcReadRequest) readRequest);
> }
>
> @SuppressWarnings("unchecked")
> default CompletableFuture<BulkPlcReadResponse> read(BulkPlcReadRequest readRequest) {
> return (CompletableFuture<BulkPlcReadResponse>) read((PlcReadRequest) readRequest);
> }
>
> @SuppressWarnings("unchecked")
> default <T> CompletableFuture<CheckedBulkPlcReadResponse<T>> read(CheckedBulkPlcReadRequest<T> readRequest) {
> return (CompletableFuture<CheckedBulkPlcReadResponse<T>>) read((PlcReadRequest) readRequest);
> }
>
> }
>
>
> The mentioned hierarchy is essential to make the overloaded methods work.
>
> As always Im happy about feedback :)
>
> Sebastian
Re: Generify branch
Posted by Sebastian Rühl <se...@googlemail.com>.
Hi Together,
I refactored the branch (refactoring/java_generify) a bit so that you now can do following:
PlcReadRequest req = new BulkPlcReadRequest()
req.addItem(new ReadRequestItem<>(Byte.class, inputs));
req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
CompletableFuture future = plcReader.read(req);
PlcReadResponse resp = (PlcReadResponse) future.get();
Only difference on the requirement from Chris is, that you now need a specific request implementation and add the <> onto the Request Items.
Currently following Types are implemented (PlcReadRequest* is a interface now):
BulkPlc(Read|Write)Re(quest|ponse)
CheckedBulkPlc(Read|Write)Re(quest|ponse)
SinglePlc(Read|Write)Re(quest|ponse)
The tag of the pre-refacotring is „pre_bulk_refactoring“ if needed for comparison (On branch refactoring/java_generify).
Essential is the hierarchy of the implementation now:
PlcReadRequest
/ \
SinglePlcReadRequest BulkPlcReadRequest
|
CheckedBulkPlcReadRequest
With this following Signatures can be used to conveniently work with strong typing:
public interface PlcReader {
CompletableFuture<? extends PlcReadResponse> read(PlcReadRequest readRequest);
@SuppressWarnings("unchecked")
default <T> CompletableFuture<SinglePlcReadResponse<T>> read(SinglePlcReadRequest<T> readRequest) {
return (CompletableFuture<SinglePlcReadResponse<T>>) read((PlcReadRequest) readRequest);
}
@SuppressWarnings("unchecked")
default CompletableFuture<BulkPlcReadResponse> read(BulkPlcReadRequest readRequest) {
return (CompletableFuture<BulkPlcReadResponse>) read((PlcReadRequest) readRequest);
}
@SuppressWarnings("unchecked")
default <T> CompletableFuture<CheckedBulkPlcReadResponse<T>> read(CheckedBulkPlcReadRequest<T> readRequest) {
return (CompletableFuture<CheckedBulkPlcReadResponse<T>>) read((PlcReadRequest) readRequest);
}
}
The mentioned hierarchy is essential to make the overloaded methods work.
As always Im happy about feedback :)
Sebastian
Re: Generify branch
Posted by Mark Keinhörster <ma...@codecentric.de>.
Perfect from my site :)
> Am 05.01.2018 um 11:01 schrieb Christofer Dutz <ch...@c-ware.de>:
>
> I agree that the changes won’t break anything and I would be fine with merging things back but we continue fine-tuning the API there.
>
> But I think if you immediately started working with that, you would probably have to adjust your branch quite often.
>
> How about merging things back from the generify branch and then working together to get things into shape (would be great if you would participate in this and help get the model into a shape good for SCALA support).
>
> Chris
>
> Am 05.01.18, 10:58 schrieb "Mark Keinhörster" <ma...@codecentric.de>:
>
> Hi,
> despite the unchecked requests, having typed request items is a great improvement.
> And as the current changes are not breaking anything it would be good to have these changes in master (so they can be applied to the Scala API without having double work)
>
> Also Chris’ idea using getValue is quite cool, let’s try to apply it :)
>
> From my perspective having the changes in master is already an improvement.
>
> Mark
>
>
>> Am 04.01.2018 um 18:47 schrieb Christofer Dutz <ch...@c-ware.de>:
>>
>> Hi Sebastian,
>>
>> What I did think about, would be to have a getValue method in the response object, which you can pass in a request item reference.
>> The method should return the value for that particular request item in the generic type the requestItem defined.
>>
>> I think the unchecked version of the Read/WriteRequest should not have the convenience constructors the generic version has. So, it should only have a no-args and a var-args Constructor that only accepts request items and eventually a list of request Items.
>>
>> And how about making the generic version extend the non-generic version? After all generics are just syntactic sugar that’s used by the compiler. So instead of setting this to the most generic type of object, I would rather opt to have the generic version extend the non-generic one with generic type and methods.
>>
>> But I’m open for other suggestions and/or opinions. So what do you all think?
>>
>> Chris
>>
>>
>>
>> Am 04.01.18, 17:36 schrieb "Sebastian Rühl" <se...@googlemail.com>:
>>
>> Hi Chris,
>>
>> Thanks for the feedback.
>>
>>> One thing I did notice, was that one ReadRequest can have different types of items inside … should the ReadRequest / WriteRequest be fixed to one generic type in both cases?
>>>
>>> Something like this:
>>>
>>> PlcReadRequest req = new PlcReadRequest();
>>> req.addItem(new ReadRequestItem(Byte.class, inputs));
>>> req.addItem(new ReadRequestItem(Boolean.class, outputs, 3));
>>> CompletableFuture future = plcReader.read(req);
>>> PlcReadResponse resp = (PlcReadResponse) future.get();
>>
>> I pushed a change so you can do this:
>> UncheckedPlcReadRequest req = new UncheckedPlcReadRequest();
>> req.addItem(new ReadRequestItem<>(Byte.class, inputs));
>> req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
>> CompletableFuture future = plcReader.read(req);
>> PlcReadResponse resp = (PlcReadResponse) future.get();
>> Without getting warnings.
>>>
>>> It does produce warnings, the way it’s currently implemented.
>>> Right now, my version allows using all types of items inside one request (even mixed), but we lost generification. Eventually it would be good to have a Read/WriteRequest and a Batch version that’s not generic.
>>
>> I tried now a couple of variants with the generics and I have found no good way to represent the batch version beside using the „unchecked“ requests.
>>
>>>
>>> What do you think? Right now, I would opt to not merge the changes back until we have discussed this fully.
>>
>> In my opinion in really depends on the usage later on. If you mainly using a single request then the generics might help. If your most use case is the UncheckedPlcReadRequest later anyway then this generic might not help at all. Maybe we can find a way to let the user define own messages and keep type safety.
>> What we could do is to split the Plc(Read|Write)Request into two classes with one single and one bulk and move the common code into an abstract plc request.
>>
>>>
>>>
>>> Chris
>>
>> Sebastian
>>
>
>
>
Re: Generify branch
Posted by Christofer Dutz <ch...@c-ware.de>.
I agree that the changes won’t break anything and I would be fine with merging things back but we continue fine-tuning the API there.
But I think if you immediately started working with that, you would probably have to adjust your branch quite often.
How about merging things back from the generify branch and then working together to get things into shape (would be great if you would participate in this and help get the model into a shape good for SCALA support).
Chris
Am 05.01.18, 10:58 schrieb "Mark Keinhörster" <ma...@codecentric.de>:
Hi,
despite the unchecked requests, having typed request items is a great improvement.
And as the current changes are not breaking anything it would be good to have these changes in master (so they can be applied to the Scala API without having double work)
Also Chris’ idea using getValue is quite cool, let’s try to apply it :)
From my perspective having the changes in master is already an improvement.
Mark
> Am 04.01.2018 um 18:47 schrieb Christofer Dutz <ch...@c-ware.de>:
>
> Hi Sebastian,
>
> What I did think about, would be to have a getValue method in the response object, which you can pass in a request item reference.
> The method should return the value for that particular request item in the generic type the requestItem defined.
>
> I think the unchecked version of the Read/WriteRequest should not have the convenience constructors the generic version has. So, it should only have a no-args and a var-args Constructor that only accepts request items and eventually a list of request Items.
>
> And how about making the generic version extend the non-generic version? After all generics are just syntactic sugar that’s used by the compiler. So instead of setting this to the most generic type of object, I would rather opt to have the generic version extend the non-generic one with generic type and methods.
>
> But I’m open for other suggestions and/or opinions. So what do you all think?
>
> Chris
>
>
>
> Am 04.01.18, 17:36 schrieb "Sebastian Rühl" <se...@googlemail.com>:
>
> Hi Chris,
>
> Thanks for the feedback.
>
>> One thing I did notice, was that one ReadRequest can have different types of items inside … should the ReadRequest / WriteRequest be fixed to one generic type in both cases?
>>
>> Something like this:
>>
>> PlcReadRequest req = new PlcReadRequest();
>> req.addItem(new ReadRequestItem(Byte.class, inputs));
>> req.addItem(new ReadRequestItem(Boolean.class, outputs, 3));
>> CompletableFuture future = plcReader.read(req);
>> PlcReadResponse resp = (PlcReadResponse) future.get();
>
> I pushed a change so you can do this:
> UncheckedPlcReadRequest req = new UncheckedPlcReadRequest();
> req.addItem(new ReadRequestItem<>(Byte.class, inputs));
> req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
> CompletableFuture future = plcReader.read(req);
> PlcReadResponse resp = (PlcReadResponse) future.get();
> Without getting warnings.
>>
>> It does produce warnings, the way it’s currently implemented.
>> Right now, my version allows using all types of items inside one request (even mixed), but we lost generification. Eventually it would be good to have a Read/WriteRequest and a Batch version that’s not generic.
>
> I tried now a couple of variants with the generics and I have found no good way to represent the batch version beside using the „unchecked“ requests.
>
>>
>> What do you think? Right now, I would opt to not merge the changes back until we have discussed this fully.
>
> In my opinion in really depends on the usage later on. If you mainly using a single request then the generics might help. If your most use case is the UncheckedPlcReadRequest later anyway then this generic might not help at all. Maybe we can find a way to let the user define own messages and keep type safety.
> What we could do is to split the Plc(Read|Write)Request into two classes with one single and one bulk and move the common code into an abstract plc request.
>
>>
>>
>> Chris
>
> Sebastian
>
Re: Generify branch
Posted by Mark Keinhörster <ma...@codecentric.de>.
Hi,
despite the unchecked requests, having typed request items is a great improvement.
And as the current changes are not breaking anything it would be good to have these changes in master (so they can be applied to the Scala API without having double work)
Also Chris’ idea using getValue is quite cool, let’s try to apply it :)
From my perspective having the changes in master is already an improvement.
Mark
> Am 04.01.2018 um 18:47 schrieb Christofer Dutz <ch...@c-ware.de>:
>
> Hi Sebastian,
>
> What I did think about, would be to have a getValue method in the response object, which you can pass in a request item reference.
> The method should return the value for that particular request item in the generic type the requestItem defined.
>
> I think the unchecked version of the Read/WriteRequest should not have the convenience constructors the generic version has. So, it should only have a no-args and a var-args Constructor that only accepts request items and eventually a list of request Items.
>
> And how about making the generic version extend the non-generic version? After all generics are just syntactic sugar that’s used by the compiler. So instead of setting this to the most generic type of object, I would rather opt to have the generic version extend the non-generic one with generic type and methods.
>
> But I’m open for other suggestions and/or opinions. So what do you all think?
>
> Chris
>
>
>
> Am 04.01.18, 17:36 schrieb "Sebastian Rühl" <se...@googlemail.com>:
>
> Hi Chris,
>
> Thanks for the feedback.
>
>> One thing I did notice, was that one ReadRequest can have different types of items inside … should the ReadRequest / WriteRequest be fixed to one generic type in both cases?
>>
>> Something like this:
>>
>> PlcReadRequest req = new PlcReadRequest();
>> req.addItem(new ReadRequestItem(Byte.class, inputs));
>> req.addItem(new ReadRequestItem(Boolean.class, outputs, 3));
>> CompletableFuture future = plcReader.read(req);
>> PlcReadResponse resp = (PlcReadResponse) future.get();
>
> I pushed a change so you can do this:
> UncheckedPlcReadRequest req = new UncheckedPlcReadRequest();
> req.addItem(new ReadRequestItem<>(Byte.class, inputs));
> req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
> CompletableFuture future = plcReader.read(req);
> PlcReadResponse resp = (PlcReadResponse) future.get();
> Without getting warnings.
>>
>> It does produce warnings, the way it’s currently implemented.
>> Right now, my version allows using all types of items inside one request (even mixed), but we lost generification. Eventually it would be good to have a Read/WriteRequest and a Batch version that’s not generic.
>
> I tried now a couple of variants with the generics and I have found no good way to represent the batch version beside using the „unchecked“ requests.
>
>>
>> What do you think? Right now, I would opt to not merge the changes back until we have discussed this fully.
>
> In my opinion in really depends on the usage later on. If you mainly using a single request then the generics might help. If your most use case is the UncheckedPlcReadRequest later anyway then this generic might not help at all. Maybe we can find a way to let the user define own messages and keep type safety.
> What we could do is to split the Plc(Read|Write)Request into two classes with one single and one bulk and move the common code into an abstract plc request.
>
>>
>>
>> Chris
>
> Sebastian
>
Re: Generify branch
Posted by Christofer Dutz <ch...@c-ware.de>.
Hi Sebastian,
What I did think about, would be to have a getValue method in the response object, which you can pass in a request item reference.
The method should return the value for that particular request item in the generic type the requestItem defined.
I think the unchecked version of the Read/WriteRequest should not have the convenience constructors the generic version has. So, it should only have a no-args and a var-args Constructor that only accepts request items and eventually a list of request Items.
And how about making the generic version extend the non-generic version? After all generics are just syntactic sugar that’s used by the compiler. So instead of setting this to the most generic type of object, I would rather opt to have the generic version extend the non-generic one with generic type and methods.
But I’m open for other suggestions and/or opinions. So what do you all think?
Chris
Am 04.01.18, 17:36 schrieb "Sebastian Rühl" <se...@googlemail.com>:
Hi Chris,
Thanks for the feedback.
> One thing I did notice, was that one ReadRequest can have different types of items inside … should the ReadRequest / WriteRequest be fixed to one generic type in both cases?
>
> Something like this:
>
> PlcReadRequest req = new PlcReadRequest();
> req.addItem(new ReadRequestItem(Byte.class, inputs));
> req.addItem(new ReadRequestItem(Boolean.class, outputs, 3));
> CompletableFuture future = plcReader.read(req);
> PlcReadResponse resp = (PlcReadResponse) future.get();
I pushed a change so you can do this:
UncheckedPlcReadRequest req = new UncheckedPlcReadRequest();
req.addItem(new ReadRequestItem<>(Byte.class, inputs));
req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
CompletableFuture future = plcReader.read(req);
PlcReadResponse resp = (PlcReadResponse) future.get();
Without getting warnings.
>
> It does produce warnings, the way it’s currently implemented.
> Right now, my version allows using all types of items inside one request (even mixed), but we lost generification. Eventually it would be good to have a Read/WriteRequest and a Batch version that’s not generic.
I tried now a couple of variants with the generics and I have found no good way to represent the batch version beside using the „unchecked“ requests.
>
> What do you think? Right now, I would opt to not merge the changes back until we have discussed this fully.
In my opinion in really depends on the usage later on. If you mainly using a single request then the generics might help. If your most use case is the UncheckedPlcReadRequest later anyway then this generic might not help at all. Maybe we can find a way to let the user define own messages and keep type safety.
What we could do is to split the Plc(Read|Write)Request into two classes with one single and one bulk and move the common code into an abstract plc request.
>
>
> Chris
Sebastian
Re: Generify branch
Posted by Sebastian Rühl <se...@googlemail.com>.
Hi Chris,
Thanks for the feedback.
> One thing I did notice, was that one ReadRequest can have different types of items inside … should the ReadRequest / WriteRequest be fixed to one generic type in both cases?
>
> Something like this:
>
> PlcReadRequest req = new PlcReadRequest();
> req.addItem(new ReadRequestItem(Byte.class, inputs));
> req.addItem(new ReadRequestItem(Boolean.class, outputs, 3));
> CompletableFuture future = plcReader.read(req);
> PlcReadResponse resp = (PlcReadResponse) future.get();
I pushed a change so you can do this:
UncheckedPlcReadRequest req = new UncheckedPlcReadRequest();
req.addItem(new ReadRequestItem<>(Byte.class, inputs));
req.addItem(new ReadRequestItem<>(Boolean.class, outputs, 3));
CompletableFuture future = plcReader.read(req);
PlcReadResponse resp = (PlcReadResponse) future.get();
Without getting warnings.
>
> It does produce warnings, the way it’s currently implemented.
> Right now, my version allows using all types of items inside one request (even mixed), but we lost generification. Eventually it would be good to have a Read/WriteRequest and a Batch version that’s not generic.
I tried now a couple of variants with the generics and I have found no good way to represent the batch version beside using the „unchecked“ requests.
>
> What do you think? Right now, I would opt to not merge the changes back until we have discussed this fully.
In my opinion in really depends on the usage later on. If you mainly using a single request then the generics might help. If your most use case is the UncheckedPlcReadRequest later anyway then this generic might not help at all. Maybe we can find a way to let the user define own messages and keep type safety.
What we could do is to split the Plc(Read|Write)Request into two classes with one single and one bulk and move the common code into an abstract plc request.
>
>
> Chris
Sebastian