Open
Conversation
Contributor
|
Thanks for working on this. re 1, I don't see harm in making that change for all cases, but I don't use the cli (I tend to just use the proc macro on its own) so I'm not very opinionated. If someone else has an opinion here I'll defer to it. re 2, Yes, both modes should be tested. We should somehow factor the test runner such that it can test more configs per LanguageMethods impl? |
Author
It was relatively easy to refactor, by removing the distinction between "test variants" and the test base case. Codegen cmd line args for the. base case were also forced upon the variants. Incidentally makes the code a little cleaner and more DRY. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
to
resolves #1347
I put this in draft mode, because there is still things to discuss.
{world_name}_impl.rs.However, I'd prefer the implementation to be in
{world_name}.rs, and the bindings file in{world_name}_bindings.rs. So I suggest to output bindings into{world_name}_bindings.rs. Even when--stubs != separate, so that the names are consistent, allowing for this use case:First time around, you run
wit-bindgen --stubs separate. Then you make manual edits to the implementation, and when the wit file changes, you just do awit-bindgen, so as not to overwrite your manual edits.As that file name change is perhaps a change users won't agree to, I wanted to discuss this first, before making that change.
--stubs separateCurrently, there only embedded stubs get codegen-tested. I have run all codegen cases for the
--stubs separatecase as well locally, but it's not a straightforward change to run tests for bothembeddedandseparate, so I have for now put the latter case behind afalseconstant:So it can at least be tested manually by flipping that const to
true. But I think we should always be testing both, because putting stubs into a different file opens a couple of different pitfalls.Please let me know what you think, what changes you'd like to see etc.
Eugen