plc4x
plc4x copied to clipboard
feat(plc4go): Implementing the correct reading of BOOL types
Implemented according to Cleanup of how we handle all the bit-related fields
Thanks for reviewing the code.
Guess I'll have to fully review this next week (Currently travelling) ... also would I like to continue the discussion on the list on if my proposal even makes sense ... cause I think we never finished that discussion. I probably should have marked that document as Work In Progress. Thing is ... I think the general notation used in many places would have more been something like BOOL[5..10] or so ...
Hi, sorry for coming late to the party ... was insanely busy on my side and I just came back from a holiday break, going through the open Pull Requests.
Could you please update your PR? I'd love to finialize it and merge it as soon as possible.
Chris
Hi Chris, welcome back. Did you mean to let me update PR for being merged smoothly or update for other things? But whatever update is needed will be a few days away, as I'm on day two of Covid-19.
@chrisdutz Hi Chris,
I have updated it, and have a commit(https://github.com/apache/plc4x/commit/17d7f765c670f86c3fd110f010a3faafe8ee1c5a) about it, please review it when you have time.
Jinlin
In general, the changes look good, however you manually seem to have edited generated code, so the changes will get lost the next time the maven build is executed.
In general, the changes look good, however you manually seem to have edited generated code, so the changes will get lost the next time the maven build is executed.
Hi Chris,
Thank you very much for reviewing the code. I have a revert commit(https://github.com/apache/plc4x/commit/8a793e26d8b24060ee657d7ca9e6114d89c724a1) of this commit(https://github.com/apache/plc4x/commit/17d7f765c670f86c3fd110f010a3faafe8ee1c5a) after Ben remind me that the Golang build failed after my push, sorry for that. The reason the Golang build failed is just what you said I edited generated file directly. But don’t worry I will be familiar with the code generation and have a PR for that as soon as possible.
Jinlin
Always happy to help educate :-)
@chrisdutz @hongjinlin as far as I can see this PR was ready to be merged? Seems then only a merge/rebase might be necessary.
No objections to merging
@hongjinlin can you merge main into this branch or rebase?
@sruehl what do you think? You want to migrate this manually or should we close and drop it?