Skip to content
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

source(from) could imply source(true) #124

Closed
tjkirch opened this issue Jul 25, 2019 · 3 comments · Fixed by #129
Closed

source(from) could imply source(true) #124

tjkirch opened this issue Jul 25, 2019 · 3 comments · Fixed by #129
Assignees
Labels
breaking change Likely requires a SemVer version bump enhancement New feature or request

Comments

@tjkirch
Copy link
Collaborator

tjkirch commented Jul 25, 2019

In #109, when considering the field attributes that can work together, there was some agreement that source(true) and source(from) make sense together on a field. source(true) indicates that a field is the source even though it can't be named "source". source(from(...)) tells Snafu how to transform another type into the type indicated in the field. (See https://docs.rs/snafu/0.4.3/snafu/guide/attributes/index.html#transforming-the-source). There's no conflict in these intentions.

Moreover, if you specify source(from) on a field, you almost certainly mean for that to imply source(true) - you've already said "source" once, and it's a conflict to specify source(false) and source(from). It's not the case today, though - the source(from) attribute would be ignored if there weren't some other signal that the field is a source.

It'd reduce line count and cognitive overhead to have source(from) imply that the field is a source. source(true) on the same field would still be valid, just redundant.

@tjkirch
Copy link
Collaborator Author

tjkirch commented Jul 25, 2019

This may be trivial to add with #109 while I'm finishing that up, but it's not strictly related to the error handling changes so I wanted to represent it as a separate issue.

tjkirch added a commit to tjkirch/snafu that referenced this issue Jul 25, 2019
@shepmaster shepmaster added breaking change Likely requires a SemVer version bump enhancement New feature or request help wanted Extra attention is needed labels Jul 25, 2019
@shepmaster
Copy link
Owner

source(true) would still be valid, just unnecessary.

To be pedantic, source(true, from(...)) would be redundant, but source(true) by itself would still be important in the case where the source field isn't named source.

@tjkirch
Copy link
Collaborator Author

tjkirch commented Jul 25, 2019

To be pedantic, source(true, from(...)) would be redundant, but source(true) by itself would still be important in the case where the source field isn't named source.

That's indeed what I meant - updated the description!

I have a commit (auto-linked above) that accomplishes this, and adds the updated compile-fail cases, a unit test, and a doc update. It's based on #109 so I was waiting for that to get merged before making an official PR here, so that I didn't need to do the dance of changing base branch. Feel free to assign this issue to me and remove the help wanted, if you like!

@shepmaster shepmaster removed the help wanted Extra attention is needed label Jul 25, 2019
@shepmaster shepmaster assigned shepmaster and tjkirch and unassigned shepmaster Jul 25, 2019
tjkirch added a commit to tjkirch/snafu that referenced this issue Jul 26, 2019
tjkirch added a commit to tjkirch/snafu that referenced this issue Jul 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Likely requires a SemVer version bump enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants