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.

Sign up for GitHub

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

Jump to bottom

Support C# onTypeFormatting in Razor LSP (Part 2) #2259

Merged
merged 3 commits into from
Aug 6, 2020
Merged

Support C# onTypeFormatting in Razor LSP (Part 2) #2259

merged 3 commits into from
Aug 6, 2020

Conversation

ajaybhargavb
Copy link
Contributor

Part of https://github.com/dotnet/aspnetcore/issues/23419

ontypeformatting2

  • Implemented a new pass based design for regular and ontype formatting
  • Added the ability to re-parse the document in FormattingContext
  • Updated FormattingVisitor and IndentationContext to have more information about each line
  • Added the core logic for C# onTypeFormatting
  • Added a FormattingContentValidatorPass to ensure the integrity of the document before returning formatting edits

Working on:

  • Adding another pass that rejects formatting edits if we format C# blocks with mixed HTML
  • Tests for everything

NTaylorMullen
NTaylorMullen reviewed Jul 23, 2020
View reviewed changes
Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Only had clarification comments / a few null check suggestions. I think the design is spot on. Really impressed by how clean and well-factored this all is. Formatting is an incredibly difficult problem to begin with and you built an API to make it as understandable as humanly possible. In the end, I somehow understood that super hard problem of formatting because of the choices you made. Bravo sir, color me impressed!

👏 💯 🥳 🎉

TanayParikh
TanayParikh approved these changes Jul 27, 2020
View reviewed changes
Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

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

👏 LGTM, just a few comments:

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/ontypeformatting2 branch from 218d138 to a4ddc1c Compare August 3, 2020 23:58
@ajaybhargavb
Copy link
Contributor Author

Updated some logic and added another validation pass. Also addressed feedback. I plan to put the test changes in a separate PR for ease of reviewing.

@ajaybhargavb
Copy link
Contributor Author

Merging this with Part 1 so I can rebase with master easily.

@ajaybhargavb ajaybhargavb marked this pull request as ready for review August 6, 2020 18:26
@ajaybhargavb ajaybhargavb requested a review from noahbkim as a code owner August 6, 2020 18:26
@ajaybhargavb ajaybhargavb merged commit bdaa03c into ajbaaska/ontypeformatting Aug 6, 2020
@ajaybhargavb ajaybhargavb deleted the ajbaaska/ontypeformatting2 branch August 6, 2020 18:26
ajaybhargavb added a commit that referenced this pull request Aug 6, 2020
@ajaybhargavb
Support C# onTypeFormatting in Razor LSP (Part 2) (#2259)
c5bdc3b 
* Core formatting logic wip

* Another validation pass and address feedback

* More feedback
ghost pushed a commit that referenced this pull request Aug 10, 2020
@ajaybhargavb
Support C# onTypeFormatting in Razor LSP (Part 1) (#2213)
22d5fc4 
* Support C# onTypeFormatting in Razor LSP

* feedback

* feedback

* Support C# onTypeFormatting in Razor LSP (Part 2) (#2259)

* Core formatting logic wip

* Another validation pass and address feedback

* More feedback

* Added tests for onTypeFormatting and friends (onTypeFormatting support part 3/3) (#2334)

* Added tests for onTypeFormatting and friends

* Feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NTaylorMullen NTaylorMullen NTaylorMullen left review comments

@TanayParikh TanayParikh TanayParikh approved these changes

@ryanbrandenburg ryanbrandenburg Awaiting requested review from ryanbrandenburg

@noahbkim noahbkim Awaiting requested review from noahbkim

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajaybhargavb @NTaylorMullen @TanayParikh