Philip Oakley <philipoakley@iee.email> writes: > Instead of replacing with "..", replace with the empty string, > implied by passing NULL, and adjust the padding length calculation. What's the point of saying "implied by passing NULL" here? Is it an excuse for passing NULL when passing "" would have sufficed and been more natural, or something? Also, it is unclear to whom you are passing the NULL. I think that it is sufficient that you said "replace with the empty string" there. > Extend the existing tests for these pretty formats to include > `Trunc` and Ltrunc` options matching the `trunc` and `ltrunc` > tests. A more important thing to say is that we add Trunc and Ltrunc, than we test for these new features ;-) You may also want to explain why there is no matching Mtrunc added. I also have another comment on the design. Imagine there are series of wide characters, each occupying two display columns, and you give 6 display columns to truncate such a string into. "trunc" would give you "[][].." (where [] denotes one such wide letter that occupies two display columns), and "Trunc" would give you "[][][]". Now if you give only 5 display columns, to fill instead of 6, what should happen? I do not recall how ".."-stuffed truncation works in this case but it should notice that it cannot stuff 3 wide letters and give you "[][].". The current code may be already buggy, but at least at the design level, it is fairly clear what the feature _should_ do. As a design question, what should "Trunc" do in such a case now? I do not think we can still call it "hard truncate" if the feature gives "[][]" (i.e. fill only 4 display columns, resulting in a string that is not wide enough) or "[][][]" (i.e. exceed 5 columns that are given), but of course chomping a letter in the middle is not acceptable behaviour, so ...
