-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Angular bracket imports for reusable reactor modules #2404
base: master
Are you sure you want to change the base?
Conversation
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.
Looks like a good start! I left some questions. The major concern I have is that there are not tests. Please add one.
@@ -99,6 +99,7 @@ public Boolean caseModel(Model object) { | |||
public Boolean caseImport(Import object) { | |||
return new ComparisonMachine<>(object, Import.class) | |||
.equalAsObjects(Import::getImportURI) | |||
.equalAsObjects(Import::getImportPackage) |
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.
Can you explain how this works?
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.
Does this effectively function like "equal to importURI OR equal to importPackage"?
timeout: 2 sec | ||
} | ||
|
||
import Count from <lib/Count.lf> |
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.
@tanneberger, tagging you because it still appears to be unclear what should be inside the angular brackets. I thought the notation was reserved for packages, not local paths?
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 is the old version. I still need to implement what we discussed last time. My focus here was to fix the formatting errors causing the check to fail. I'm currently working on a new version where we shorten the paths inside the angular brackets to <root/file.lf>
for downloaded Lingo packages and lib/file.lf
for locally defined libraries
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.
Do you mean "lib/file.lf"
for locally defined libraries? My assumption had been that angular brackets are only for published packages, not locally defined ones.
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.
Exactly, use angular brackets for published packages and quotation marks for locally defined libraries. In my previous comment, I was referring to "lib/file.lf"
, but I mistakenly omitted the quotation marks
…d test case for federated LF program using new imports
…inal; Refined importPackage management across affected files
…s in federated LF programs
This PR proposes a new import statement syntax to distinguish between user-defined reactors import and reusable reactors imports. The new syntax for importing reusable reactors will use angular brackets, as shown:
import Reactor from <path/to/lib/Reusables.lf>
.