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

Bury Error::description() #2230

Merged
merged 4 commits into from May 3, 2018

Conversation

@kornelski
Contributor

kornelski commented Nov 29, 2017

Rendered

Thread on internals

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa
Contributor

nagisa commented Nov 29, 2017

Show outdated Hide outdated 0000-bury-description.md Outdated
@jnicholls

This comment has been minimized.

Show comment
Hide comment
@jnicholls

jnicholls Nov 30, 2017

Totally agree on making this method optional/transparent/forgotten/deprecated/dismissed/booted/chopped/kicked/etc.

jnicholls commented Nov 30, 2017

Totally agree on making this method optional/transparent/forgotten/deprecated/dismissed/booted/chopped/kicked/etc.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Feb 14, 2018

Member

@withoutboats can you shepherd this RFC?

Member

aturon commented Feb 14, 2018

@withoutboats can you shepherd this RFC?

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Feb 14, 2018

Contributor

Why not deprecate it also?

Contributor

withoutboats commented Feb 14, 2018

Why not deprecate it also?

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Feb 14, 2018

Contributor

@withoutboats It might be just me... but .description() seems like a cheap way to describe error enums with unit-only-variants... There's always Debug, but a &str is cheaper..

To make .description() a bit more useful you could perhaps make it derivable?

Contributor

Centril commented Feb 14, 2018

@withoutboats It might be just me... but .description() seems like a cheap way to describe error enums with unit-only-variants... There's always Debug, but a &str is cheaper..

To make .description() a bit more useful you could perhaps make it derivable?

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Feb 14, 2018

Member

@Centril can you point to some code that uses description in that way?

Member

sfackler commented Feb 14, 2018

@Centril can you point to some code that uses description in that way?

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Feb 14, 2018

Contributor

@sfackler Not really; If you have such a unit-variant enum, then I guess you can use an inherent method instead - the bound isn't important wrt. .description() here, and neither as a trait object.

Contributor

Centril commented Feb 14, 2018

@sfackler Not really; If you have such a unit-variant enum, then I guess you can use an inherent method instead - the bound isn't important wrt. .description() here, and neither as a trait object.

kornelski added a commit to kornelski/book that referenced this pull request Apr 20, 2018

@U007D

This comment has been minimized.

Show comment
Hide comment
@U007D

U007D Apr 20, 2018

Overall, I like this. Although I'm not sure why we'd steer people away from implementing .description() in documentation and not officially deprecate it. The rationale given:

... this RFC does not propose formal deprecation at this time to avoid unnecessary warnings during the transition.

would seem to apply any time any feature is deprecated, no? The above-mentioned warning would also serve to reinforce the updated documentation, essentially ensuring discovery.

Overall, things are currently a bit messier than I think we'd all like around errors and error handling--moving as swiftly as we can (but not recklessly) seems to me to be a good way to realizing the full potential of Rust's error handling--to that end, I'd (also) support official deprecation of .description().

Procedural question: Should I review with "change requested", or just leave this feedback as it is?

U007D commented Apr 20, 2018

Overall, I like this. Although I'm not sure why we'd steer people away from implementing .description() in documentation and not officially deprecate it. The rationale given:

... this RFC does not propose formal deprecation at this time to avoid unnecessary warnings during the transition.

would seem to apply any time any feature is deprecated, no? The above-mentioned warning would also serve to reinforce the updated documentation, essentially ensuring discovery.

Overall, things are currently a bit messier than I think we'd all like around errors and error handling--moving as swiftly as we can (but not recklessly) seems to me to be a good way to realizing the full potential of Rust's error handling--to that end, I'd (also) support official deprecation of .description().

Procedural question: Should I review with "change requested", or just leave this feedback as it is?

@repax

This comment has been minimized.

Show comment
Hide comment
@repax

repax Apr 20, 2018

I would like description() to be deprecated, or at least just return "". I think it's undesirable to expose identifier names (which are just source-code level entities).

The identifier name of a type should not have any consequence on its public output/effect - especially not by default.

repax commented Apr 20, 2018

I would like description() to be deprecated, or at least just return "". I think it's undesirable to expose identifier names (which are just source-code level entities).

The identifier name of a type should not have any consequence on its public output/effect - especially not by default.

@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Apr 21, 2018

Contributor

What should the default message be? Type name is out. Empty string is out.

Contributor

kornelski commented Apr 21, 2018

What should the default message be? Type name is out. Empty string is out.

@repax

This comment has been minimized.

Show comment
Hide comment
@repax

repax Apr 21, 2018

Is is out of the question to have different outputs depending on whether dbg_print is enabled? If enabled, a more detailed description could be provided by the compiler, and if not a minimal default would be used. The developer can always add his/her own implementation, if necessary.

The default output for both the dbg_print and the non-dbg_print case could simply be left unspecified. That should be compatible with the current situation.

repax commented Apr 21, 2018

Is is out of the question to have different outputs depending on whether dbg_print is enabled? If enabled, a more detailed description could be provided by the compiler, and if not a minimal default would be used. The developer can always add his/her own implementation, if necessary.

The default output for both the dbg_print and the non-dbg_print case could simply be left unspecified. That should be compatible with the current situation.

@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Apr 21, 2018

Contributor

I'm thinking about making the message push authors towards switching to Display instead, so "description is deprecated" or "use Display instead" or "https://rust-lang.org/how-to-fix-descriptions"

Contributor

kornelski commented Apr 21, 2018

I'm thinking about making the message push authors towards switching to Display instead, so "description is deprecated" or "use Display instead" or "https://rust-lang.org/how-to-fix-descriptions"

@U007D

This comment has been minimized.

Show comment
Hide comment
@U007D

U007D Apr 21, 2018

Hmm.. this is tricky! :)

".description() is deprecated--see https://rust-lang.org/how-to-fix-description" (along with explanatory content at that address) seems to be a reasonable default message to me, given the excellent points made by @repax and @Mark-Simulacrum. (Suggest singular 'description' in URL)

U007D commented Apr 21, 2018

Hmm.. this is tricky! :)

".description() is deprecated--see https://rust-lang.org/how-to-fix-description" (along with explanatory content at that address) seems to be a reasonable default message to me, given the excellent points made by @repax and @Mark-Simulacrum. (Suggest singular 'description' in URL)

bors added a commit to rust-lang/rust that referenced this pull request Apr 30, 2018

Auto merge of #50163 - kornelski:error, r=Kimundi
Bury Error::description()

Second attempt of #49536 rust-lang/rfcs#2230

The exact wording of the default implementation is still up in the air, but I think it's a detail that can be amended later.
@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Apr 30, 2018

Contributor

the implementation is merged!

done

Contributor

kornelski commented Apr 30, 2018

the implementation is merged!

done

Show outdated Hide outdated 0000-bury-description.md Outdated
@U007D

U007D approved these changes Apr 30, 2018

@U007D

U007D approved these changes Apr 30, 2018

@U007D

This comment has been minimized.

Show comment
Hide comment
@U007D

U007D Apr 30, 2018

I have no power... :)

U007D commented Apr 30, 2018

I have no power... :)

@alexcrichton alexcrichton merged commit 01434ea into rust-lang:master May 3, 2018

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 3, 2018

Member

It turns out that the implementation for this RFC has already merged! In light of that I'm going to go ahead and merge the RFC

Member

alexcrichton commented May 3, 2018

It turns out that the implementation for this RFC has already merged! In light of that I'm going to go ahead and merge the RFC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment