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

Should lpad/rpad be defined in terms of textwidth instead of code units or have the option to do so? #38256

Closed
KristofferC opened this issue Nov 1, 2020 · 7 comments · Fixed by #39044
Labels
good first issue Indicates a good issue for first-time contributors to Julia minor change Marginal behavior change acceptable for a minor release strings "Strings!"

Comments

@KristofferC
Copy link
Member

One of the main use cases of lpad and rpad is, at least for me personally, to align things in the terminal. However, lpad and rpad are defined (and documented) to work in terms of code units (i.e. they call length on the input string) which means that when length and textwidth disagree, you get unaligned output:

julia> s1 = "⟨k|H₁|k̃⟩"
"⟨k|H₁|k̃⟩"
julia> s2 = "⟨k|H₁|k⟩"
"⟨k|H₁|k⟩"
julia> textwidth(s1), textwidth(s2)
(8, 8)
julia> rpad(s1, 12) |> textwidth
11
julia> rpad(s2, 12) |> textwidth
12

This can cause bugs like KristofferC/TimerOutputs.jl#94.

Two points of discussions:

  • Should the default be changed to use textwidth instead of length? Perhaps too much of a breaking change.
  • Should there be an option to use textwidth, e.g. with a kwarg?
@KristofferC KristofferC added the strings "Strings!" label Nov 1, 2020
@rfourquet
Copy link
Member

I have seen functions like mylpad in a package to workaround this behavior... So yeah, at least providing an option to use textwidth would be great.

Does someone know the usecase for using length instead of textwidth ?

@KristofferC KristofferC added the triage This should be discussed on a triage call label Nov 1, 2020
@vtjnash
Copy link
Member

vtjnash commented Nov 12, 2020

Triage said textwidth sounds best here

@vtjnash vtjnash removed the triage This should be discussed on a triage call label Nov 12, 2020
@JeffBezanson
Copy link
Member

From triage: we can't think of a use case where you would want length.

@StefanKarpinski StefanKarpinski added the minor change Marginal behavior change acceptable for a minor release label Nov 12, 2020
@stevengj stevengj added the good first issue Indicates a good issue for first-time contributors to Julia label Nov 16, 2020
@rohanxyzg
Copy link

Hey,
I would like to work on this issue.
Thanks!

@StefanKarpinski
Copy link
Member

Go for it!

@KristofferC
Copy link
Member Author

@rohanxyzg, Just a heads up since you post this in a few "good first issue" posts, there is generally no need to "claim" an issue, you can just start working on it and submit a PR when it is finished :).

@guptatanmay8
Copy link

Hi, can you help me out with how should I approach this issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Indicates a good issue for first-time contributors to Julia minor change Marginal behavior change acceptable for a minor release strings "Strings!"
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants