Even better model panics #1721
Even better model panics #1721
Conversation
|
I'm not experienced with CI so I have to ask, these checks that seem to run with every commit I make, they don't cost you extra money every time I trigger them, right? I'm working on fixing that merge conflict. |
|
Thank you for the pull request, @IamTheCarl! I'll take a look soon (most likely later today). A note: I generally prefer rebase over merge to fix conflicts, because all those merge commits end up making the pull request harder to review for me. A merge tends to make more sense, if I've already reviewed a pull request, but more changes were necessary (because then I can just ignore the previous commits and focus on the new commits when I review again). This is documented in
It doesn't. CI resources are provided by GitHub for free, fortunately. It still makes sense to keep CI usage down, to not waste resources, to not compete with other ongoing builds (number of concurrent builds per project is restricted, I believe), and not least for your own convenience (CI turnaround times are annoyingly long). You can do most of what the CI build ends up doing locally, by running |
|
Ah, I forgot to comment on something.
Yeah, don't worry about that. It's better to have stack traces, because once we have them, we can improve upon them. I already have some ideas, like expandable status messages (#1496). |
There was a problem hiding this comment.
Thank you for the pull request, @IamTheCarl, looks good!
I left one comment that I'd like to see addressed. And since I'm already requesting changes, I don't see that leftover debug message being removed. That would be nice too.
Other than that, this is good to go!
crates/fj-proc/src/lib.rs
Outdated
| let attrs = item.attrs; | ||
| let vis = item.vis; | ||
| let sig = item.sig; | ||
| let statements = item.block.stmts; | ||
|
|
||
| let item = quote::quote! { | ||
| #(#attrs)* #vis #sig { | ||
| fj::abi::initialize_panic_handling(); | ||
| #(#statements)* | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
You seem to be adding the new and improved code here, without removing the old and busted one. initialize_panic_handling says it can be called multiple times, but I think that's not enough reason to keep the old code around 😂
There was a problem hiding this comment.
I Had removed the old version and that println but for some reason merging brought a lot of the old stuff back. I missed those two.
I'm going to have to give this re-basing thing a try.
I'll try to fix that up today.
|
Looks good to go now! Thank you, @IamTheCarl! |
|
Hmm, this pull request was no longer up-to-date with the |
Finishes off the last changes needed to resolve #1569
Yes it even cleans up the syn AST stuff.
My only complaint is that the stack trace is really long but I don't think there's much we can do about that at this time.