Skip to content

Conversation

@funkygao
Copy link

before this fix, the output was not aligned correctly

before this fix, the output was not aligned correctly
@ryanuber
Copy link
Owner

Hey @funkygao, thanks for the PR. I have a few concerns about this:

  1. Performance is a factor, as I mentioned in a (recently) closed PR here. This logic has to pass over each character. I have not had the time to run the benchmark against this branch myself, but I am guessing there will be a good difference here, and I'm trying to avoid negating all of the performance optimization we did in Optimize format time #10. One thought would be to completely prune out the color codes prior to checking the length with a pre-compiled regular expression or similar. As I mentioned in the above linked comment, we might need to provide an option to enable/disable the color code detection so that systems which produce huge amounts of output don't become slow as a result.

  2. I am not convinced that m invariably indicates the end of a color sequence code. I'll admit I am not an expert in this area, but IIRC there is an escaping / terminal width calculation problem which can manifest if the color code is not enclosed in \[\]. We should be certain that this is the right way to detect the end of the color code for that case.

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.

2 participants