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

refactor: miette::Error as Diagnostic and rspack_error::Error #4892

Merged
merged 10 commits into from
Dec 8, 2023

Conversation

h-a-n-a
Copy link
Contributor

@h-a-n-a h-a-n-a commented Dec 5, 2023

Summary

Based on the overall idea behind this in #4866.

This PR leverages miette and thiserror for representing errors and diagnostics.
This makes aligning with webpack errors easily.

  1. miette::Error as rspack_error::Error
  2. miette::Error as Diagnostic
  3. Result<T> = std::result::Result<T, miette::Error>
  4. Print diagnostics with miette::GraphicalReportHandler
  5. Eagerly deduped ecma errors on site, and remove PartialEq, etc from Diagnostics.
  6. Refactored one Webpack Error to diagnostic representation, use https://github.com/web-infra-dev/rspack/pull/4892/files#diff-b6b8b1bb36b909de2bbe46c5de8c4c34515cb1c9710e8d02faec369b352da525R167 as an example

Minor changes:

  1. Removed clippy::unwrap_in_result as previously discussed.

Test Plan

See snapshot changes.

Require Documentation?

  • No
  • Yes, the corresponding rspack-website PR is __

@h-a-n-a h-a-n-a mentioned this pull request Dec 5, 2023
2 tasks
@github-actions github-actions bot added the team The issue/pr is created by the member of Rspack. label Dec 5, 2023
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_io_errors branch from 3406e7f to 371fc68 Compare December 5, 2023 05:07
@h-a-n-a h-a-n-a changed the title refactor: io errors refactor: merge everything to miette Dec 5, 2023
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_error_and_diagnostics branch from 4c359da to 35be7b3 Compare December 5, 2023 05:12
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_io_errors branch 3 times, most recently from 61772dc to e6d3be0 Compare December 5, 2023 07:33
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_error_and_diagnostics branch from 35be7b3 to 2247eee Compare December 5, 2023 07:58
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_io_errors branch 4 times, most recently from f9ad11f to d8388ff Compare December 5, 2023 13:14
@h-a-n-a

This comment was marked as outdated.

@rspack-bot

This comment was marked as outdated.

@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_io_errors branch 4 times, most recently from ad87ad0 to 0143b8b Compare December 6, 2023 08:07
@h-a-n-a h-a-n-a mentioned this pull request Dec 6, 2023
2 tasks
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_io_errors branch 4 times, most recently from c046417 to 0894cfe Compare December 7, 2023 10:21
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_error_and_diagnostics branch from 2247eee to b6ff92e Compare December 7, 2023 10:57
@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_io_errors branch 2 times, most recently from 618339d to c867026 Compare December 7, 2023 13:58
IWANABETHATGUY
IWANABETHATGUY previously approved these changes Dec 8, 2023
ahabhgk
ahabhgk previously approved these changes Dec 8, 2023
Base automatically changed from 12-04-refactor_error_and_diagnostics to main December 8, 2023 06:06
@h-a-n-a h-a-n-a dismissed stale reviews from ahabhgk and IWANABETHATGUY December 8, 2023 06:06

The base branch was changed.

Copy link
Contributor Author

h-a-n-a commented Dec 8, 2023

Merge activity

  • Dec 8, 1:08 AM: Graphite rebased this pull request after merging its parent, because this pull request is set to merge when ready.
  • Dec 8, 1:28 AM: @@h-a-n-a merged this pull request with Graphite.

@h-a-n-a h-a-n-a force-pushed the 12-04-refactor_io_errors branch from 7109327 to c55a30c Compare December 8, 2023 06:08
@h-a-n-a h-a-n-a merged commit 8bfd363 into main Dec 8, 2023
15 of 26 checks passed
@h-a-n-a h-a-n-a deleted the 12-04-refactor_io_errors branch December 8, 2023 06:28
ahabhgk pushed a commit that referenced this pull request Jan 2, 2024
…4892)

<!--
  Thank you for submitting a pull request!

  We appreciate the time and effort you have invested in making these changes. Please ensure that you provide enough information to allow others to review your pull request.

  Upon submission, your pull request will be automatically assigned with reviewers.

  If you want to learn more about contributing to this project, please visit: https://github.com/web-infra-dev/rspack/blob/main/CONTRIBUTING.md.
-->

## Summary

Based on the overall idea behind this in #4866. 

This PR leverages `miette` and `thiserror` for representing errors and diagnostics.
This makes aligning with webpack errors easily. 

1. `miette::Error` as `rspack_error::Error`
2. `miette::Error` as `Diagnostic`
3. `Result<T> = std::result::Result<T, miette::Error>`
4. Print diagnostics with `miette::GraphicalReportHandler`
5. Eagerly deduped ecma errors on site, and remove `PartialEq`, etc from `Diagnostics`.
6. Refactored one Webpack Error to diagnostic representation, use https://github.com/web-infra-dev/rspack/pull/4892/files#diff-b6b8b1bb36b909de2bbe46c5de8c4c34515cb1c9710e8d02faec369b352da525R167 as an example

Minor changes:
1. Removed `clippy::unwrap_in_result` as previously discussed.

<!-- Can you explain the reasoning behind implementing this change? What problem or issue does this pull request resolve? -->

<!-- It would be helpful if you could provide any relevant context, such as GitHub issues or related discussions. -->

## Test Plan

See snapshot changes.

<!-- Can you please describe how you tested the changes you made to the code? -->

## Require Documentation?

<!-- Does this PR require documentation? -->

- [X] No
- [ ] Yes, the corresponding rspack-website PR is \_\_
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team The issue/pr is created by the member of Rspack.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants