-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bugfixes in handling of target properties across imports #2232
Conversation
@lhstrh I also tried to fix #2083 in this PR. I made lingua-franca/core/src/main/java/org/lflang/generator/c/CGenerator.java Lines 725 to 734 in 5997ca5
I'd appreciate it if you give me a more detailed direction. What validation should be done? |
Thanks for taking a look at this, @byeong-gil. The issue is a bit tricky.
In a way, each target property already specifies what its update behavior is, through its implementation of |
I had a look, but it doesn't seem we're actually dealing with target properties from imported files in unfederated programs (it's only addressed in |
Oh, I see. The reason why "it works" is because of the stuff mentioned in #2083. A better way to do this would be to create general a mechanism for getting updates from imported files. The way I would probably go about doing this would be to create some functions in
I would guess that |
In order to honor |
You're right. I didn't deal with unfederated programs because I wasn't sure whether I was in the right way. Now I understand what you want to do. Thanks, @lhstrh! To clarify, I wanna ask one more question. For what situation |
My understanding it should be used to determine which properties to load from the federate itself (as opposed to the federation). Specifically, in the constructor of
while the following should be filtered by
Does this make sense? |
Oh, so it's the federate version of |
I think that a federate itself can also have imports, which would also need to be handled... |
Hmm... Then I don't understand yet. Actually, the below code handles imported federate.
Regarding the below file, the timeout property from the federation is In
In
|
Right, so that suggests that the |
By the way, I would look in the |
Ok, I'll analyze the code more and fix it. Thanks for your help! |
900a6bd
to
fea23a1
Compare
@lhstrh I attempted to fix this issue again. Now, I have two questions.
|
This looks great, @byeong-gil. Let me make a pass over this and push changes that will help me answer your question. I believe you got us 90% there already. |
This is taking a little longer than expected. One problem I see is that some things are handled in |
WalkthroughThe changes primarily enhance configuration merging, resource handling, and property loading mechanisms across the federated, generator, and target properties components. Key improvements include using lambdas for configurable property loading, refining resource collection and inheritance handling, and adding new methods to streamline importing and extending target properties. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FederateTargetConfig
participant GeneratorBase
participant TargetConfig
participant CGenerator
participant GeneratorUtils
User->>FederateTargetConfig: Call mergeImportedConfig with lambda
FederateTargetConfig->>TargetConfig: merge and update configurations
User->>GeneratorBase: generate code
GeneratorBase->>TargetConfig: load and process properties
User->>CGenerator: generate C code
CGenerator->>GeneratorUtils: load and collect resources
Assessment against linked issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (10)
Files skipped from review due to trivial changes (1)
Additional comments not posted (18)
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
87d0a3a
to
be6f740
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a considerable improvement! I left a few small comments.
Co-authored-by: Christian Menard <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
This PR fixes bugs in the handling of target properties across imports. It addresses #2083; fixes #2149; fixes #2286.
files
target property #2333Summary by CodeRabbit
New Features
Refactor
List
toSet
for improved handling.Bug Fixes
Documentation
Tests