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

Diagnostic console output #236

Merged
merged 11 commits into from
Aug 24, 2022
Merged

Conversation

ReeceHumphreys
Copy link
Contributor

This PR adds better console when displaying diagnostics to users.
To test it you will have to update Cargo.toml in icerpc-csharp to point to this branch. As such, I have added two screenshots of the console output.

One screenshot shows the output before the changes and one shows the output after the changes made in this PR.

I welcome any design ideas or changes we want to make!

Screen Shot 2022-08-24 at 9 17 51 AM

Screen Shot 2022-08-24 at 9 15 54 AM

@ReeceHumphreys ReeceHumphreys added the enhancement New feature or request label Aug 24, 2022
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
Copy link
Member

@externl externl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@InsertCreativityHere InsertCreativityHere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found alot of this hard to follow, I think the code would benefit from a couple comments explaining what each section does.

src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
@InsertCreativityHere
Copy link
Member

Also, there's a 'bug' with the underling of multiline errors, that I mentioned to you in person that you should fix before merging.

ReeceHumphreys and others added 3 commits August 24, 2022 14:02
Co-authored-by: Austin Henriksen <austin.r.henriksen79@gmail.com>
Copy link
Member

@InsertCreativityHere InsertCreativityHere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just minor things, mostly ownership improvements.

src/slice_file.rs Outdated Show resolved Hide resolved
src/slice_file.rs Outdated Show resolved Hide resolved
src/slice_file.rs Outdated Show resolved Hide resolved
src/slice_file.rs Outdated Show resolved Hide resolved
src/slice_file.rs Outdated Show resolved Hide resolved
src/slice_file.rs Outdated Show resolved Hide resolved
src/slice_file.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
src/parse_result.rs Outdated Show resolved Hide resolved
ReeceHumphreys and others added 2 commits August 24, 2022 15:41
Co-authored-by: Austin Henriksen <austin.r.henriksen79@gmail.com>
@ReeceHumphreys ReeceHumphreys merged commit 11c81ca into icerpc:main Aug 24, 2022
@ReeceHumphreys ReeceHumphreys deleted the error-output branch August 24, 2022 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants