-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Extend error message #960
Merged
Merged
Extend error message #960
Changes from 27 commits
Commits
Show all changes
40 commits
Select commit
Hold shift + click to select a range
a0b2e96
Initial
captain-yossarian ad35203
formatting
captain-yossarian d41d466
wip
captain-yossarian 5ca1685
add util fn
captain-yossarian 0538813
message update
captain-yossarian 166e4b4
refactor
captain-yossarian 1516923
update error logic
captain-yossarian 1cefeae
message refactor
captain-yossarian a056c89
remove debug flag
captain-yossarian e0b0684
update
captain-yossarian bc3b08c
add stderr
captain-yossarian 69f0e0a
update condition
captain-yossarian 8ea567c
CR fix
captain-yossarian 81edf1b
remove empty line
captain-yossarian ae76a38
update
captain-yossarian 3546d81
update error message
captain-yossarian b0cef02
wip
captain-yossarian 9c69526
work
captain-yossarian 145393a
unchange clippy
captain-yossarian 6f307f9
update
captain-yossarian f7681e5
remove logs
captain-yossarian 990437e
println
captain-yossarian f421174
format
captain-yossarian 23bdf72
clippy
captain-yossarian 62bb054
VERY WIP
captain-yossarian 8fdbc69
error collect
captain-yossarian bf42a1a
refactor
captain-yossarian b9b2a6d
add second argument
captain-yossarian ee82df4
format
captain-yossarian 6ee8610
wip
captain-yossarian 0ff9648
wip with main parsser
captain-yossarian 53bd8ce
wip
captain-yossarian a619b5b
receive unexpected error
captain-yossarian 8a4f067
Fix with props checks
jstarry b55d572
merge
captain-yossarian 748c62d
remove comments
captain-yossarian b8c8a38
update
captain-yossarian 5b64ce8
format
captain-yossarian 114cb16
Merge remote-tracking branch 'origin' into issue/664
captain-yossarian 7fd2382
Fix test
jstarry 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
Oops, something went wrong.
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.
@jstarry What do you think about refactor of this function ?
It looks like I have to apply same conditions for both WithProps and ListProps logic.
So, I created a HashMap with all edge cases.
For example: if you want apply only
ref
edge case, you can pass a slice with one element "ref" as a second argument toapply_edge_cases
. This let you use same logic for bothListProps
andWithProps
.If in the future, you would like to add more adge cases, you can only insert new
[key, valye]
to the hash map.I just don't want to copy/paste logic from one function to another.
Also, here you can concatenate all errors in one string and output it to the user.
If you think it is not Ok, you can check this
23bdf72
commit.Here I just copied logic from one function to another, it covers all edge cases and it is not DRY
P.S. The code you see is a draft, it is not my final PR
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.
@captain-yossarian nice! Here are my thoughts:
Good point, how about we pull that common logic out of
WithProps
andListProps
and handle itProps
instead? I'm imagining something like:with props
I'm not convinced this is really helping much, I think the for loop and if statements that we had before were clearer. We don't need to generalize this too much.
Yeah, generally it's good to avoid copy/paste logic. I think in this case, we can avoid it by handling common paths at the
Props
levelI think one error at a time is fine. Also, you can collect a list of results into one: https://doc.rust-lang.org/std/result/enum.Result.html#impl-FromIterator%3CResult%3CA%2C%20E%3E%3E
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.
With this approach, I think we can allow
ref
to come beforewith props
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.
@jstarry I have a problem with props type.
return_type
function can catch all kind of errors. It works perfectly.But it works only with
let _ = Props::collect_props(input)?;
433 lineIf I remove this line, app throw an error in this place:
return Err(input.error("MY"));
464 line.Do you have any thoughts why does it is happening?
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.
@captain-yossarian yeah the issue is that you are consuming the
ParseStream
inget_type
and so there are no tokens left to parse. Look at howPeek
implementations work. They use a cursor which can read without consuming any tokens from theParseStream
.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.
@jstarry not quite. Here I making a fork. If you log
input.cursor().token_stream().to_string()
at the end ofget_type
you will get all initial stream