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

bugfix: Work around fansi escaping issues #731

Merged
merged 2 commits into from
Dec 18, 2022

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented Dec 15, 2022

Previously, some error messages might have been not able to be printed due to a bug in fansi com-lihaoyi/fansi#46

Now, we added a workaround that should help out here.

Fixes #730

Previously, some error messages might have been not able to be printed due to a bug in fansi com-lihaoyi/fansi#46

Now, we added a workaround that should help out here.

Fixes scalameta#730
@tgodzik tgodzik requested review from keynmol and ckipp01 December 15, 2022 18:29
Copy link

@khajavi khajavi left a comment

Choose a reason for hiding this comment

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

Thank you! 🙏

@keynmol
Copy link
Collaborator

keynmol commented Dec 15, 2022

@tgodzik would it be easy to add a test for this?

I assume we can throw an exception with a broken fansi string in a markdown document?

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

👍🏼 looks good, but agree with @keynmol, is it possible to include a test for this?

@tgodzik tgodzik force-pushed the workaround-fansi branch 5 times, most recently from 90cab27 to a1dde47 Compare December 16, 2022 14:40
@tgodzik tgodzik requested a review from ckipp01 December 16, 2022 16:21
@tgodzik
Copy link
Contributor Author

tgodzik commented Dec 16, 2022

👍🏼 looks good, but agree with @keynmol, is it possible to include a test for this?

Alright, I finally managed to get the test working. I also moved the CliSuite to tests, since it was not being even run at all 😨

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM!

@tgodzik tgodzik merged commit f67e6fb into scalameta:main Dec 18, 2022
@tgodzik tgodzik deleted the workaround-fansi branch December 18, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants