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: improve tsc diagnostics #7420

Merged
merged 5 commits into from
Sep 12, 2020

Conversation

kitsonk
Copy link
Contributor

@kitsonk kitsonk commented Sep 11, 2020

This PR removes a significant amount of logic around TypeScript diagnostics out of the compiler and moves it Rust. Also, it refactors the diagnostics in Rust to be self-contained and more align to idiomatic Rust using fmt.

There is one stylistic change, in that when displaying a line of code, there is a blank line between the message and the code, which makes the message more readable.

JSError can be refactored to align and further improve the Rust side as a seperate PR.

@nayeemrmn
Copy link
Collaborator

I don't think the extra new line is more readable with multiple errors since they are only separated a single line. Right now they appear neatly as blocks:
image
Plus any extraneous new lines are a bit more of a pain for thin integrated terminal views.

Can we instead give a prefix to the source line?

@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 11, 2020

I really don't want to get into an ad nauseam debate, so I will simply revert the stylistic change.

@kitsonk
Copy link
Contributor Author

kitsonk commented Sep 12, 2020

@bartlomieju finally have the first part of many, took me longer than I thought.

It adds 118 lines (of non test code) to diagnostics.rs but removes 200 lines of JavaScript from the compiler and makes the whole thing a lot cleaner. I also think a further rewrite to JSError will cleanup even more of the code out of fmt_errors.rs.

@kitsonk kitsonk mentioned this pull request Sep 12, 2020
13 tasks
@bartlomieju
Copy link
Member

@kitsonk LGTM, I will land this PR after #7432

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @kitsonk

@bartlomieju bartlomieju merged commit 10fbfcb into denoland:master Sep 12, 2020
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.

None yet

3 participants