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

Should error when source/backtrace(false) given on wrong field #140

Closed
tjkirch opened this issue Aug 7, 2019 · 2 comments · Fixed by #156
Closed

Should error when source/backtrace(false) given on wrong field #140

tjkirch opened this issue Aug 7, 2019 · 2 comments · Fixed by #156
Assignees
Labels
breaking change Likely requires a SemVer version bump enhancement New feature or request good first issue Good for newcomers

Comments

@tjkirch
Copy link
Collaborator

tjkirch commented Aug 7, 2019

source(false) should only be accepted on fields named "source", and backtrace(false) should only be accepted on fields named "backtrace". Otherwise, the user has likely misinterpreted their purpose.

This is a small follow-up to the misuse errors added in #109 for issue #105.

@shepmaster shepmaster added breaking change Likely requires a SemVer version bump enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Aug 7, 2019
@tjkirch
Copy link
Collaborator Author

tjkirch commented Aug 8, 2019

If someone is interested in jumping on this, here's what I'd recommend:

  • In snafu-derive/src/lib.rs, scan through fn parse_snafu_enum to get a sense of the error handling, particularly where you see errors.add with a OnlyValidOn
  • Where you see the "opt_out" variables being set, we want to check if the field name (name) is what we expect before flipping the boolean
  • Add compile-fail tests for each of the attributes that will now fail (look in compatibility-tests/compile-fail/tests/ui)

If no one bites, I'd like to do it before 0.5 since it's related to the other error message improvements and should go in the same breaking-change release :)

@tjkirch tjkirch mentioned this issue Aug 8, 2019
7 tasks
tjkirch added a commit to tjkirch/snafu that referenced this issue Aug 9, 2019
@tjkirch tjkirch removed the help wanted Extra attention is needed label Aug 9, 2019
@tjkirch tjkirch self-assigned this Aug 9, 2019
@tjkirch
Copy link
Collaborator Author

tjkirch commented Aug 9, 2019

I've got this ready in tjkirch@70d974b but I can't make a PR until #141 is in -- GitHub won't let me create a PR in this repo against a branch in my fork. Without that ability, a PR would also show all of #141. (The implementation is a fair bit different with #141, so I didn't see a point in implementing pre-#141.)

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 good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants