-
Notifications
You must be signed in to change notification settings - Fork 98
Add an include keyword
#174
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
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.
Could you expand a bit about the semantic meaning of the include statement? For example when with is required, what with does, when include might fail, etc.
Good suggestion. I have thought about that but then I saw the section I've modified is for Lexical Structure. Is there a dedicated section for semantic structure of WIT? If not, I can expand the the semantic meaning in the part where I modified. @alexcrichton |
|
The top of the document is probably the best place for that right now, probably around the area that |
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.
Looking good, but I might recommend trying to run some examples through wit-parser to sort out self/pkg paths since right now they're relatively imprecise and can lead to some ambiguities in the examples I think. I realize that include isn't implemented yet but I think most other pieces should be able to be fleshed out to make sure they're valid.
As I think about this as well though the "de-duplication" section I think will actually need to be required for the first implementation. It's not valid to import the same interface twice under two names, so if the same interface is imported into two worlds which are union'd then it's required that a deduplication happens rather than emitting two separate imports (e.g. resolving with with isn't valid)
That's a really good point. Transient deps are always hard haha. I will update the doc to include de-dep semantics. |
|
~one way to make this less verbose than: ... could be to change the Of course, this would mostly just be syntactical sugar – the union of worlds should still equate to a world. |
|
@alexcrichton thanks for reviewing. Could you please take another look? |
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 good to me, thanks!
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: Jiaxiao Zhou <jiazho@microsoft.com>
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
Signed-off-by: jiaxiao zhou <jiazho@microsoft.com>
|
I will rebase once #198 is merged in. |
This commit updates the WIT syntax in all of the examples illustrating the use of the include syntax. Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
|
Okay, I think this PR is ready to review @guybedford |
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.
It would be great to see this moving forward, thanks for the update.
Co-authored-by: Guy Bedford <guybedford@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
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.
LGTM
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.
Awesome work, Joe!
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
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.
Thanks for the updates, and great work. Regarding the most recent change, which says that you can't rename an ID, unfortunately we can't just rule it out syntactically, because include x with { a as b } may or may not be allowed depending on x. Here's a suggested rewording, but feel free to reject and do it your own way.
Co-authored-by: Luke Wagner <mail@lukewagner.name>
Co-authored-by: Luke Wagner <mail@lukewagner.name>
|
Thank you, @lukewagner !! I have committed your suggestions. |
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.
Thanks Joe, overall lgtm, thanks for spearheading this! I'm thinking I'll merge early next week if there aren't any more comments from anyone.
|
Thanks everyone for reviewing this!! This meant a lot to me as it marks the first contribution I've contributed to a Wasm proposal. Cheers 🙌 🥂 |
As proposed by #169 , this PR adds an
includekeyword to the WIT.md. It also sorts the keyword list alphabetically.