-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add matching pattern to error in fields #76903
Merged
Merged
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
8c9cccc
Add matching pattern to error in fields
nik9000 e3ed135
More tests
nik9000 1814043
Merge branch 'master' into fields_error_include_name
nik9000 78b1cba
update skip
nik9000 e9e05ea
Revert "update skip"
nik9000 3dfc5ce
Revert
nik9000 f10c0dc
Merge branch 'master' into fields_error_include_name
nik9000 830f0c1
Updates
nik9000 0f15553
Revert more
nik9000 d65037c
More revert
nik9000 ac7de99
Merge branch 'master' into fields_error_include_name
nik9000 10c8ef5
Revert "Revert "update skip""
nik9000 fd67f2c
Merge branch 'master' into fields_error_include_name
nik9000 be7c84b
check
nik9000 9434fec
Merge branch 'master' into fields_error_include_name
nik9000 8b98a9b
These work now!
nik9000 06f448f
No such luck
nik9000 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of piecing exception messages together in this way (in particular because
IllegalArgumentException
is a generic exception).Also, it's unclear to me how the change in this PR will affect other call sites of the valueFetcher method (those will now not return the field name anymore and might be less precise / harder to debug).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were running this in production myself I wouldn't do it, no. But I think its useful because we drop the
getMessage()
from the top level cause into thereason
field of the error by default. I really care that thereason
describes exactly what failed here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the other call sites - I believe there is only one non-test call that doesn't flow through here and it is in highlighting which calls the method in a way that won't throw. But the method is public and, who knows, highlighting may throw. I'll revert this and be happy with the more verbose message. It isn't worth that change.