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

A fill command to replace str lpad and str rpad #7846

Merged
merged 12 commits into from Feb 9, 2023
Merged

A fill command to replace str lpad and str rpad #7846

merged 12 commits into from Feb 9, 2023

Conversation

fdncred
Copy link
Collaborator

@fdncred fdncred commented Jan 23, 2023

Description

The point of this command is to allow you to be able to format ints, floats, filesizes, and strings with an alignment, padding, and a fill character, as strings. It's meant to take the place of str lpad and str rpad.

> help fill
Fill and Align

Search terms: display, render, format, pad, align

Usage:
  > fill {flags}

Flags:
  -h, --help - Display the help message for this command
  -w, --width <Int> - The width of the output. Defaults to 1
  -a, --alignment <String> - The alignment of the output. Defaults to Left (Left(l), Right(r), Center(c/m), MiddleRight(cr/mr))
  -c, --character <String> - The character to fill with. Defaults to ' ' (space)

Signatures:
  <number> | fill -> <string>
  <string> | fill -> <string>

Examples:
  Fill a string on the left side to a width of 15 with the character '─'
  > 'nushell' | fill -a l -c '─' -w 15

  Fill a string on the right side to a width of 15 with the character '─'
  > 'nushell' | fill -a r -c '─' -w 15

  Fill a string on both sides to a width of 15 with the character '─'
  > 'nushell' | fill -a m -c '─' -w 15

  Fill a number on the left side to a width of 5 with the character '0'
  > 1 | fill --alignment right --character 0 --width 5

  Fill a filesize on the left side to a width of 5 with the character '0'
  > 1kib | fill --alignment middle --character 0 --width 10

image

User-Facing Changes

Deprecated str lpad and str rpad.

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

@fdncred fdncred added the pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes label Jan 25, 2023
@bobhy
Copy link
Contributor

bobhy commented Jan 26, 2023

Maybe this is a good place to address #7757 ? And the hangover from #7752?

Since this new command is all about the graphic rendition, I'd say users will expect, e.g -w 10 to output 10 visible characters on the screen (if input is shorter than that). So in this context, I assert users will not be surprised to find fill is counting grapheme clusters by default (even if it's the other way for the new str length -g of #7752). Likewise (and this is the bee in my bonnet), I don't think users would be surprised to see fill not counting ANSI escape sequences (because this command context is about display, not string processing).

Unicode combining character:

> let b = $"(char -u 1F468 200D 1F466 200D 1F466)"
> $b
👨‍👦‍👦
> $b | fill -a l -c '-' -w 7
👨‍👦‍👦-

I'd expect that to be 👨‍👦‍👦------

ANSI escapes:

〉let a = $"(ansi red)abc(ansi green)def(ansi reset)"
〉$a
abcdef
〉$a | fill -a l -c '-' -w 20
abcdef---
>

I'd expect abcdef-------------- here.

I've been looking for a place to put this capability and could take a crack at working it into your fill command, but don't want to trample on your PR. Do you think these are worthwhile changes in the first place? and if so, how should we integrate them?

@fdncred
Copy link
Collaborator Author

fdncred commented Jan 26, 2023

@bobhy Feel free to add your changes in this PR if github will allow you to do that.

@fdncred
Copy link
Collaborator Author

fdncred commented Jan 31, 2023

@bobhy I was wondering if you were going to add changes to this PR or if you want to land it and tweak after that?

@bobhy
Copy link
Contributor

bobhy commented Jan 31, 2023 via email

@sholderbach
Copy link
Member

storming in with the plans for the bikeshed

I am not quite fond off the name fill. To me that sounds more like a command to fill some missing values or a whole column etc. Among the str subcommands a str fill would fit in a bit more in my view.

(Personally I liked the explicit subcommands over flags with some possible values a bit more but I can see the added power/flexibility of this approach as well)

@fdncred
Copy link
Collaborator Author

fdncred commented Feb 1, 2023

Hey @sholderbach. The intent with this command is that:

  1. It handles more than strings, which is why it's not named as a str command (although, in practice that's more of a novelty, although some may find it helpful maybe).
  2. It's more than padding, although it does pad. It's meant to align as well so if someone was trying to center text within a certain terminal width, it's trivial to do with this. That's really the only new functionality.
  3. The original idea was to use rust's formatting symbology which include width, fill & alignment, sign, and precision but there didn't see a way to programmatically combine all those.

Having said, all that. I'm up for a rename to str fill or we could also close this and add a str centerpad or some such thing to handle the alignment that's missing now.

I'm also not going to get my feelings hurt if we just close this PR. Just looking to consolidate some things.

@bobhy
Copy link
Contributor

bobhy commented Feb 1, 2023

Hi @fdncred, I think there is good value in this approach and have given up on trying to shoehorn formatting directives into string variable interpolation.
So I'm interested in adding in the grapheme cluster counting (and ANSI escape skipping) logic and submitting this, should be able to complete it in the next day or so. However, I can't modify your PR in any case, it will have to come from my own repo (but sharing author credit to you). Are you OK with that?
Hi @sholderbach, The name "fill" is unfortunate, but "format" is already taken. Calling it 'fmt' would be just hostile. But the essence of the command is about counting print positions on the screen, which means counting grapheme clusters, not chars, and not counting ANSI escape sequences, sez me. This kind of arithmetic would be highly confusing in a 'str' subcommand,

@fdncred
Copy link
Collaborator Author

fdncred commented Feb 9, 2023

I'm holding my breath while pushing this button. Here goes nothing!

@fdncred fdncred merged commit 00601f1 into nushell:main Feb 9, 2023
@fdncred fdncred deleted the fill_command branch February 9, 2023 20:56
fdncred pushed a commit that referenced this pull request Feb 20, 2023
…I escape codes (#8134)

Enhancement of new `fill` command (#7846) to handle content including
ANSI escape codes for formatting or multi-code-point Unicode grapheme
clusters.
In both of these cases, the content is (many) bytes longer than its
visible length, and `fill` was counting the extra bytes so not adding
enough fill characters.

# Description

This script:
```rust
# the teacher emoji `\u{1F9D1}\u{200D}\u{1F3EB}` is 3 code points, but only 1 print position wide.
echo "This output should be 3 print positions wide, with leading and trailing `+`"
$"\u{1F9D1}\u{200D}\u{1F3EB}" | fill -c "+" -w 3 -a "c"

echo "This output should be 3 print positions wide, with leading and trailing `+`"
$"(ansi green)a(ansi reset)" | fill -c "+" -w 3 -a c
echo ""
```

Was producing this output:
```rust
This output should be 3 print positions wide, with leading and trailing `+`
🧑‍🏫

This output should be 3 print positions wide, with leading and trailing `+`
a
```

After this PR, it produces this output:
```rust
This output should be 3 print positions wide, with leading and trailing `+`
+🧑‍🏫+

This output should be 3 print positions wide, with leading and trailing `+`
+a+
```
# User-Facing Changes

Users may have to undo fixes they may have introduced to work around the
former behavior. I have one such in my prompt string that I can now
revert.

# Tests + Formatting

Don't forget to add tests that cover your changes.
-- Done

Make sure you've run and fixed any issues with these commands:

- [x] `cargo fmt --all -- --check` to check standard code formatting
(`cargo fmt --all` applies these changes)
- [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect` to check that you're using the standard code
style
- [x] `cargo test --workspace` to check that all tests pass

# After Submitting

`fill` command not documented in the book, and it still talks about `str
lpad/rpad`. I'll fix.

Note added dependency on a new library `print-positions`, which is an
iterator that yields a complete print position (cluster + Ansi sequence)
per call. Should this be vendored?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:breaking-change This PR implies a change affecting users and has to be noted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants