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

Consistently reset color formatting #3034

Merged
merged 4 commits into from
Feb 8, 2024

Conversation

thaliaarchi
Copy link
Contributor

Before, arrays would not reset colors after [ and ,, but objects would; it would reset colors twice before ] and }; and some cases of indentation would have colors applied. Now, colors are reset immediately after any token that is colored, before any indentation. This makes the formatting consistent, for the benefit of custom JQ_COLORS.

Before, arrays would not reset colors after `[` and `,`, but objects
would; it would reset colors twice before `]` and `}`; and some cases of
indentation would have colors applied. Now, colors are reset immediately
after any token that is colored, before any indentation. This makes the
formatting consistent, for the benefit of custom `JQ_COLORS`.
} > $d/expect
cmp $d/color $d/expect

## Set non-default colors, complex input, indented
Copy link
Member

@wader wader Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this regression test verify the reset issue for arrays? i can visually see the issue using JQ_COLORS='0;30:0;31:0;32:0;33:0;34:7;35:1;36:1;37' jq -Cn '[{"a":true,"b":false},123,null]' (7 to reverse colors for arrays)

Nice tests 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file

Copy link
Member

@wader wader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let's wait for one more review

Copy link
Member

@emanuele6 emanuele6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you!

@thaliaarchi
Copy link
Contributor Author

thaliaarchi commented Feb 7, 2024

The only remaining inconsistency, is that : is one unit for coloring. So that space would be the only one with a background color. I could make it clear before the colon. I don’t have a strong preference either way, but now I’m leaning towards clear between colon and space. Do we want the background to fill the space between the key and value or just be behind the colon?

I’ll push that fix later in the day.

@emanuele6 emanuele6 self-requested a review February 7, 2024 12:54
@emanuele6
Copy link
Member

For future reference
file

@thaliaarchi
Copy link
Contributor Author

Thanks for the screenshots. They've inspired me to make my own, and here's the color formatting over the relevant commits:

JQ_COLORS='48;5;89:48;5;21:48;5;28:48;5;207:48;5;51:48;5;9:48;5;208:48;5;11' \
jq -n '[{"a":true,"b":false},"abc",123,null]'
color-reset

Colors have a color separate from object keys, so were not tested.
@thaliaarchi
Copy link
Contributor Author

I'm all done on my end and am ready to merge.

Here's a before/after recap:

< ␛[1;35m[\n
> ␛[1;35m[␛[0m\n
<   ␛[1;36m{\n
>   ␛[1;36m{␛[0m\n
<     ␛[0m␛[1;37m"a"␛[0m␛[1;36m: ␛[0m␛[0;32mtrue␛[0m␛[1;36m,\n
>     ␛[1;37m"a"␛[0m␛[1;36m:␛[0m ␛[0;32mtrue␛[0m␛[1;36m,␛[0m\n
<     ␛[0m␛[1;37m"b"␛[0m␛[1;36m: ␛[0m␛[0;31mfalse␛[0m␛[1;36m\n
>     ␛[1;37m"b"␛[0m␛[1;36m:␛[0m ␛[0;31mfalse␛[0m\n
<   ␛[1;36m}␛[0m␛[1;35m,\n
>   ␛[1;36m}␛[0m␛[1;35m,␛[0m\n
<   ␛[0;34m"abc"␛[0m␛[1;35m,\n
>   ␛[0;34m"abc"␛[0m␛[1;35m,␛[0m\n
<   ␛[0;33m123␛[0m␛[1;35m,\n
>   ␛[0;33m123␛[0m␛[1;35m,␛[0m\n
<   ␛[0;30mnull␛[0m␛[1;35m\n
>   ␛[0;30mnull␛[0m\n
< ␛[1;35m]␛[0m\n
> ␛[1;35m]␛[0m\n

Before:

␛[1;35m[\n
  ␛[1;36m{\n
    ␛[0m␛[1;37m"a"␛[0m␛[1;36m: ␛[0m␛[0;32mtrue␛[0m␛[1;36m,\n
    ␛[0m␛[1;37m"b"␛[0m␛[1;36m: ␛[0m␛[0;31mfalse␛[0m␛[1;36m\n
  ␛[1;36m}␛[0m␛[1;35m,\n
  ␛[0;34m"abc"␛[0m␛[1;35m,\n
  ␛[0;33m123␛[0m␛[1;35m,\n
  ␛[0;30mnull␛[0m␛[1;35m\n
␛[1;35m]␛[0m\n

After:

␛[1;35m[␛[0m\n
  ␛[1;36m{␛[0m\n
    ␛[1;37m"a"␛[0m␛[1;36m:␛[0m ␛[0;32mtrue␛[0m␛[1;36m,␛[0m\n
    ␛[1;37m"b"␛[0m␛[1;36m:␛[0m ␛[0;31mfalse␛[0m\n
  ␛[1;36m}␛[0m␛[1;35m,␛[0m\n
  ␛[0;34m"abc"␛[0m␛[1;35m,␛[0m\n
  ␛[0;33m123␛[0m␛[1;35m,␛[0m\n
  ␛[0;30mnull␛[0m\n
␛[1;35m]␛[0m\n

Generated roughly with:

#!/usr/bin/env bash
set -euo pipefail

for ref in 8a9a74d ccebd8e; do
  git checkout "$ref"
  make -j8
  mv jq "$(git describe --tags)"
done

colors() {
  JQ_COLORS='0;30:0;31:0;32:0;33:0;34:1;35:1;36:1;37' \
  "$1" -Cn '[{"a":true,"b":false},"abc",123,null]' |
    gsed -z -e 's/\n/\\n\n/g' -e 's/\x1b/␛/g'
}
diff <(colors ./jq-1.7.1-18-g8a9a74d) \
     <(colors ./jq-1.7.1-21-gccebd8e)

thaliaarchi added a commit to thaliaarchi/jqjq that referenced this pull request Feb 7, 2024
Updates to match the changes in jqlang/jq#3034.
Copy link
Member

@wader wader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nicowilliams nicowilliams merged commit 54cc15c into jqlang:master Feb 8, 2024
28 checks passed
@nicowilliams
Copy link
Contributor

Thanks!

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.

4 participants