-
Notifications
You must be signed in to change notification settings - Fork 44
refactor: splicer WIT and generated bindings #252
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
refactor: splicer WIT and generated bindings #252
Conversation
462326e to
53a37ac
Compare
This commit refactors the WIT for the splicer, and moves the generated bindings to their own module file which is likely a bit easier to grok for most, and a good place to put extra functionality (e.g. custom impls for the generated types).
53a37ac to
7c99f0e
Compare
tschneidereit
left a comment
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, modulo nits.
For future reference, I found this very hard to review because almost all of the changes are stylistic in nature and unrelated to the PR's description. I appreciate that while working on a functional change it's very common to feel the need to apply refactorings like this, and don't want to dissuade you from following that. What I'd prefer is if you'd always split them out into separate commits, and mention that fact in the PR. As long as you ensure that those separate commits truly don't include any functional changes, that allows reviewers to skim them much more quickly, and apply more dedicated focus on the actually important changes.
|
And now I see that a lot of these changes come from #248. That would have been really really good to know when reviewing this. |
|
Hey @tschneidereit yeah so this one is the most recent PR -- the stylistic changes are from the introduction of clippy in #248 -- I should have been more explicit about the PR order. I see how you got to this PR first, the order in this comment is backwards |
|
Yeah my apologies there -- maybe what I can do here is just make the rest of them Drafts until the previous ones have landed. Stacked commits don't really work on forks, but leaving them in Draft and leaving a note should have been enough to give a better heads up. |
|
Leaving stacked PRs in draft certainly works, but just giving an explicit heads-up should also be enough. Just requiring reviewers to have a global view of all PRs when reviewing a single one isn't great. |
This commit refactors the WIT for the splicer, and moves the generated bindings to their own module file which is likely a bit easier to grok for most, and a good place to put extra functionality (e.g. custom impls for the generated types).
The code from this pr is split out from #247 for easy reviewing