Allow the context selector struct name to be specified #210
Conversation
This file contains 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
You could already specify that no context selector should be generated, using context(false). Now you can specify the name of the context selector, using context(MySelectorTypeName).
Can you explain a bit more about the general path you see? I don't follow how this plays into that bigger feature. |
shepmaster
reviewed
May 23, 2020
| Sometimes, you may already have a struct with the same name as one of | ||
| the variants of your error enum. You might also have 2 error enums | ||
| that share a variant. In those cases, there may be naming collisions | ||
| with the context selector type(s) generated by Snafu, which by default |
There was a problem hiding this comment.
The proper (non-code) name of the library is "SNAFU".
| enum Error { | ||
| #[snafu(context(FooContext))] | ||
| Foo { | ||
| source: std::io::Error |
There was a problem hiding this comment.
Suggested change
| source: std::io::Error | |
| source: std::io::Error, |
Comment on lines
+24
to
+27
| #[snafu( | ||
| context(InvalidUserContext), | ||
| display = r#"("User ID {} is invalid", user_id)"# | ||
| )] |
There was a problem hiding this comment.
This style is fine, which is why it compiles, but I just find it ugly for whatever reason. FWIW, I tend to split it into two:
#[snafu(context(InvalidUserContext))]
#[snafu(display = r#"("User ID {} is invalid", user_id)"#)]| }, | ||
| #[snafu( | ||
| context(SaveConfigContext), | ||
| display = r#"("Could not open config file at {}", source)"# |
There was a problem hiding this comment.
Feel free to use the modern style in these tests:
display("Could not open config file at {}", source)| #[derive(Debug, Snafu)] | ||
| enum Error { | ||
| #[snafu( | ||
| context(OpenConfigContext), |
There was a problem hiding this comment.
Seeing it in use, I wonder if we want to give some more "namespacing". For example:
context(name(OpenConfigContext)),Or
context(selector(OpenConfigContext)),Or
context(selector_name(OpenConfigContext)),
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.
You could already specify that no context selector should be generated, using
context(false). Now you can specify the name of the context selector, usingcontext(MySelectorTypeName).Helps with #188. I mean, it doesn't do exactly what asked there, which was to be able to put the generated context selectors into a submodule, but in the comments of that issue it was suggested) that this could be an alternative solution for the same issues.
This is also the first step toward #199, where custom context selectors will be needed.