Closed engineerjoe440 closed 2 years ago
Surface-level I think this is looking good - thanks as always for the contribution!
I'd like to do a bit of a deeper dive before merging, though. Bear with me, though, I'm not sure I'll have the time today.
Surface-level I think this is looking good - thanks as always for the contribution!
I'd like to do a bit of a deeper dive before merging, though. Bear with me, though, I'm not sure I'll have the time today.
Please, don't worry about this not "happening today." I'm very appreciative for your incredible responsiveness thus far! Thank you!
I completely understand wanting to go through this a bit more. Please let me know what questions you have! I want to make sure you feel comfortable with any of my contributions!
I had a bit of time this morning and found there were a few things that could be simplified. In the grammar, function_call
and fb_invocation
are grammatically identical, but there have always been two classes to represent each of them.
From context alone, I don't think it's possible to differentiate between a function call and a function block call (feel free to correct me if I'm wrong). So to that end, I pushed a commit up to a separate branch 32780d9 that removes fb_invocation
and replaces it with function_call
.
There are a couple minor fixes in there along with some reminders about why those properties are there.
I also ran the test suite against more local PLC code repositories, so I think this all should be good. So if that looks/sounds good to you, cherry-pick to your branch and we can move forward with this. 👍
OK! I think that I've pulled that in correctly, but please let me know if there's anything I've missed. Thank you!
Looks good! Going to click that green merge button.
Changes:
Closing Thoughts:
I'm not absolutely sure that I'm following the correct practice in regards to the addition of
FunctionCall
in theStructureElementDeclaration
see here. That said, I'm happy to make any adjustments as you see fit! I'm sorry that I couldn't find better references for the use of access specifiers for functions and type declarations. It seems that feature is not very widely documented.