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.

Sign up for GitHub

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

Jump to bottom

uucore: add alacritty to the list of terminals that support colors #6725

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

uucore: add alacritty to the list of terminals that support colors #6725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
785e0ac
uucore: add alacritty to the list of terminals that support colors
BuriedInTheGround Sep 21, 2024
0187ee4
dircolors: patch test fixture to include alacritty support
BuriedInTheGround Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/uu/dircolors/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,16 @@ dircolors -b > /PATH_TO_COREUTILS/tests/fixtures/dircolors/bash_def.expected
dircolors -c > /PATH_TO_COREUTILS/tests/fixtures/dircolors/csh_def.expected
```

Apply the patches to include more terminals that support colors:

```shell
git apply /PATH_TO_COREUTILS/src/uu/dircolors/alacritty-supports-colors.patch
```

Comment on lines +12 to +17
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this step necessary? The change is already applied by this commit, so this instruction is meaningless.

Suggestion: Remove this instruction, and remove the patchfile.

Copy link
Author

Choose a reason for hiding this comment

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

It is necessary because this README explains how to update the fixtures from the internal database, so the next person who does it should also do this step. Otherwise, the changes will be lost and the tests will fail again next time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would changes in git be lost?

Copy link
Author

Choose a reason for hiding this comment

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

If one updates the test fixtures, why wouldn't they commit the changes? They will, obviously, and this patch would be lost. Therefore, we need to tell them to re-apply it.
It seems quite a simple matter to me. Maybe I'm not expressing myself well, if so, please tell me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible that you didn't notice that the change to the fixture is already in the commit? https://github.com/uutils/coreutils/pull/6725/files#diff-26d0675eb8206b8804aec078728efbfb465417b19b82bf606811a217f9580bc3

Copy link
Author

Choose a reason for hiding this comment

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

I know, I made the commit. 😅

Okay. Let me try to explain this with an example.

  1. Our fellow developer, let's call them Alice, notices something odd in uutils dircolors.
  2. They make some changes to fix the behavior.
  3. At this point, they want to run the tests to make sure they didn't break anything.
  4. Alice is a fan of dircolors and knows that a new version of the GNU counterpart has just been released.
  5. They recall seeing the README of uutils dircolors, titled "How to update the internal database".
  6. Since a new release of GNU dircolors is out, they decide to follow the steps to update the test fixtures.
  7. Now, they run the tests and... A test is failing!
  8. However, it is not Alice's fault; their code did not actually break anything.

So why did a test fail? The problem is that, at step 6, Alice updated the test fixtures overwriting the patch that adds Alacritty to the list of terminals that support colors.

Conclusion: We should tell Alice to apply the patch every time they regenerate the test fixtures. Also, we kindly provide them the patch file.

Run the tests:

```shell
cargo test --features "dircolors" --no-default-features
```

Edit `/PATH_TO_COREUTILS/src/uu/dircolors/src/colors.rs` until the tests pass.
Edit `/PATH_TO_COREUTILS/src/uu/dircolors/src/dircolors.rs` until the tests pass.
12 changes: 12 additions & 0 deletions src/uu/dircolors/alacritty-supports-colors.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
diff --git a/tests/fixtures/dircolors/internal.expected b/tests/fixtures/dircolors/internal.expected
index e151973f2..01dae4273 100644
--- a/tests/fixtures/dircolors/internal.expected
+++ b/tests/fixtures/dircolors/internal.expected
@@ -7,6 +7,7 @@
# restrict following config to systems with matching environment variables.
COLORTERM ?*
TERM Eterm
+TERM alacritty*
TERM ansi
TERM *color*
TERM con[0-9]*x[0-9]*
1 change: 1 addition & 0 deletions src/uucore/src/lib/features/colors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
/// restrict following config to systems with matching environment variables.
pub static TERMS: &[&str] = &[
"Eterm",
"alacritty*",
"ansi",
"*color*",
"con[0-9]*x[0-9]*",
Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/dircolors/internal.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
# restrict following config to systems with matching environment variables.
COLORTERM ?*
TERM Eterm
TERM alacritty*
TERM ansi
TERM *color*
TERM con[0-9]*x[0-9]*
Expand Down
Loading