Honor --color option when emitting errors from parse session #5726
Conversation
There was a problem hiding this comment.
@Sindisil thanks for the PR and your first contribution to rustfmt 🎉
I checked out your branch and also validated that these changes work as expected!
I'm happy to move forward with the PR as is, but I'll give @calebcartwright a chance to look this over as well.
I'm also not sure how easy it would be to write a test for this, but if it's possible that would be great.
|
@ytmimi - Thanks for such quick feedback on my PR! I had looked at providing a test, but honestly wasn't sure how to do so. I don't think there's a way to capture stderr during test to verify presence or lack of color codes, nor am I familiar enough with the moving parts to know of a way to query a parse session to see if its error emitter supports color. I dug around a bit, but didn't find an obvious way. If anyone could point me in the right direction, I'd be more than happy to put together a test for this. I'll also try to find time to do some more digging on my own. |
|
@Sindisil this is definitely a tricky test to write. I can come up with a hacky way to do it, but it might be more trouble than it's worth. After spending some time thinking about it, I think it's alright if we don't include a test for it. |
|
i'm not too worried about a test for this either, certainly not as a prerequisite for moving forward. this seems like a straightforward bug fix, so likely want to just pull it into the next release. feel free to create a new issue to keep tabs on potentially adding a test down the road |
calebcartwright
added
pr-ready-to-merge
release-notes
There was a problem hiding this comment.
lgtm but @ytmimi will defer to you to merge since you'd previously indicated potentially having another ask (also you can just let me know if you want me to merge to override the updated branch rule, as this won't have conflicts with any subsequently merged changes)
Fixes issue 5717.
Pass color option into default_handler() and use that to determine the ColorConfig to use when creating EmitterWriter for stderr in parse session in the case where stderr supports color.
Fixes #5717.