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

[Super errors] Remove dependency on Format for the code frame display #5013

Merged
merged 3 commits into from
Mar 19, 2021

Conversation

chenglou
Copy link
Member

[Super errors] Remove dependency on Format for the code frame display

This is a mostly feature-compatible refactor of super_misc, which is used to display the code frame in super errors reporting. I've optimized, and added useless features, to keep the tests mostly still intact so that this can be merged more comfortably. After this gets merged, I'll clean up some more logic.

Changes:

  • No more Format.
  • No more need to patch compiler for colors (will do this in the next diffs).
  • Less need to understand the intricacies of coloring. It's now mostly encapsulate within this module.

This is a mostly feature-compatible refactor of super_misc, which is used to display the code frame in super errors reporting. I've optimized, and added useless features, to keep the tests mostly still intact so that this can be merged more comfortably. After this gets merged, I'll clean up some more logic.

Changes:
- No more Format.
- No more need to patch compiler for colors (will do this in the next diffs).
- Less need to understand the intricacies of coloring. It's now mostly encapsulate within this module.
@bobzhang
Copy link
Member

If you are sure of no regressions I am fine. Note the benefit is unclear since Format is heavily used in the compiler codebase which seems impossible to be eliminated

@chenglou
Copy link
Member Author

Yeah I'm not trying to eliminate Format for the sake of it. Just that it'll reduce some maintenance burden all around since this doesn't need compiler patches and other copy pasting concerns for the syntax repo anymore.

Regarding regressions: the tests have a decent coverage, though I couldn't find an error from us that needs recursive error reporting.

@chenglou chenglou merged commit 39db854 into rescript-lang:master Mar 19, 2021
@chenglou chenglou deleted the se-no-format branch March 19, 2021 10:30
chenglou added a commit to chenglou/rescript-compiler that referenced this pull request Mar 19, 2021
This is technically a small bug from way back. The recent refactor rescript-lang#5013 allowed us to fix this.
chenglou added a commit that referenced this pull request Mar 19, 2021
This is technically a small bug from way back. The recent refactor #5013 allowed us to fix this.
chenglou added a commit to chenglou/rescript-compiler that referenced this pull request Mar 19, 2021
chenglou added a commit to chenglou/rescript-compiler that referenced this pull request Mar 19, 2021
Fixes part of rescript-lang/rescript-vscode#86

Previous to rescript-lang#5013 we couldn't fix this issue because we used Format to break lines for us. Now that we're controlling this manually, we can draw the gutter for those lines.
chenglou added a commit that referenced this pull request Mar 19, 2021
Fixes part of rescript-lang/rescript-vscode#86

Previous to #5013 we couldn't fix this issue because we used Format to break lines for us. Now that we're controlling this manually, we can draw the gutter for those lines.
chenglou added a commit to chenglou/rescript-compiler that referenced this pull request Mar 19, 2021
chenglou added a commit that referenced this pull request Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants