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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

printer: implement rewrapping #423

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

declanvong
Copy link
Contributor

@declanvong declanvong commented Oct 7, 2021

Aims to resolve dprint/dprint-plugin-typescript#52 and issues similar to it. Not production ready code, it's just a hack to see if it would work :) if the idea is sound, then i'll definitely be relying on you to implement it properly 馃槃

See the test added here: https://github.com/dprint/dprint-plugin-typescript/pull/307/files#diff-43adf1891c4524ba7c0351e00d2e649ecc1211d4b14256d22aeb6f7c184193ceR1

@dsherret
Copy link
Member

@declanvong sorry for my huge delay on this. I just rebased this (can't push to your branch because I don't have permission) and tried it out. It seems much better! I'm thinking we should add this here to the printer, but I want to try it out in larger codebases though.

@declanvong
Copy link
Contributor Author

sounds good! In that case, I can test it out in our repo first to do a double check. I'll also add you as a collaborator on my fork so that you can push straight up to the branch.

@declanvong
Copy link
Contributor Author

I'll also try to get some performance numbers on our repo -- I'm curious as to how much the additional save points + restores will affect total runtime. Hopefully it's a rare enough case that it doesn't impact anything majorly though.

@declanvong declanvong marked this pull request as ready for review December 11, 2021 23:15
@dsherret dsherret force-pushed the declan/rewrap-poc branch 2 times, most recently from 82b56f9 to b962bdd Compare December 11, 2021 23:21
@CLAassistant

This comment has been minimized.

@dsherret
Copy link
Member

dsherret commented Dec 11, 2021

Yeah, I failed at fixing the commit author 馃槄 (I've given up because it will be resolved once squash merged)

Edit: Fixed.

@declanvong
Copy link
Contributor Author

Ran it over our codebase -- looking good still, after the rebase! Performance also looks no worse (in the few runs I tested, some were faster and some were slower, so it looks like any difference is negligible and down to run-by-run variance)

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.

Code requires format twice in scenario with long union type in interface extends clause
3 participants