Skip to content

Derive Debug impls #78

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

Merged
merged 2 commits into from
May 10, 2019
Merged

Derive Debug impls #78

merged 2 commits into from
May 10, 2019

Conversation

lnicola
Copy link
Contributor

@lnicola lnicola commented Apr 19, 2019

Not sure this is an improvement, but hey, at least it fixes a TODO.

I also sneaked in a Cargo.toml change to avoid a warning.

Copy link
Contributor

@Pauan Pauan left a 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!

@Pauan
Copy link
Contributor

Pauan commented Apr 20, 2019

One thing I would suggest is to increase the version of wasm-bindgen to 0.2.41, since that's the version that introduced Debug for Closure.

@lnicola lnicola force-pushed the debug-impls branch 2 times, most recently from 7a511a3 to 218399e Compare April 20, 2019 06:16
@lnicola
Copy link
Contributor Author

lnicola commented Apr 20, 2019

Filed rustwasm/wasm-bindgen#1477 for the JsFuture impl.

I'm actually not sure that this PR is a good change, since it makes the debug output more crowded while saving a couple of lines of code.

@Pauan
Copy link
Contributor

Pauan commented Apr 20, 2019

Debug isn't really intended to be "clean", it's intended for situations where you're debugging problems.

So it's usually better to err on the side of providing more information.

@lnicola
Copy link
Contributor Author

lnicola commented Apr 20, 2019

So it's usually better to err on the side of providing more information.

Sure, but if it's always going to print a JsFuture { ... } field, it doesn't bring anything of value. I see it like Sender.

@Pauan
Copy link
Contributor

Pauan commented Apr 20, 2019

It does provide value though: it lets you know that it's implemented with JsFuture (so you can then go and look up the docs/implementation for that).

Also, not using derive(Debug) is a refactoring hazard, since somebody might forget to update the Debug impl when making changes to the type.

@lnicola lnicola changed the title Derive Debug impls [blocked] Derive Debug impls Apr 27, 2019
@lnicola lnicola changed the title [blocked] Derive Debug impls Derive Debug impls May 2, 2019
@lnicola
Copy link
Contributor Author

lnicola commented May 2, 2019

This is now ready, methinks.

@Pauan Pauan requested a review from fitzgen May 10, 2019 16:21
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fitzgen fitzgen merged commit b6b73ad into rustwasm:master May 10, 2019
@lnicola lnicola deleted the debug-impls branch May 10, 2019 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants