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

fix lpad/rpad for zero-textwidth padding #41133

Closed
wants to merge 3 commits into from
Closed

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jun 8, 2021

Hopefully fixes test failure caused by backwards-incompatible change in #39044. Alternative to #41131.

@stevengj stevengj added strings "Strings!" bugfix This change fixes an existing bug labels Jun 8, 2021
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Jun 8, 2021
@StefanKarpinski
Copy link
Member

The only part I'm unsure about is switching to measuring the string's size with length when the padding has textwidth zero. It is the most backwards compatible, but it feels a bit iffy to me. I guess it does signal that you are not intending to use the function in the "text width mode".

@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jun 9, 2021
@DilumAluthge
Copy link
Member

DilumAluthge commented Jun 9, 2021

CI should get fixed on master with #41142, so there's no immediate rush on this.

@stevengj
Copy link
Member Author

stevengj commented Jun 9, 2021

One could speculate that anyone who is using lpad or rpad to pad with \0 to a fixed number of "characters" probably actually wanted a fixed number of bytes (as in the Tar.jl case), so their old code was problematic (buggy if there is any possibility of non-ASCII data) and perhaps it is better to throw an error.

On the other hand, more backwards compatibility is good too in 1.x, and from that perspective anyone explicitly using lpad(s, n, '\0') would probably rather get the same answer as in 1.6 than an error ("darn it, I know my s is ASCII") .

@vtjnash vtjnash added DO NOT MERGE Do not merge this PR! and removed bugfix This change fixes an existing bug DO NOT MERGE Do not merge this PR! labels Jun 9, 2021
@stevengj
Copy link
Member Author

So what is the conclusion here?

@vtjnash
Copy link
Member

vtjnash commented Jun 10, 2021

This is probably surprising and non-reliable behavior, so we probably want to stick with the current behavior (since the Tar bug is now fixed)

@stevengj
Copy link
Member Author

Okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants