Skip to content

Beautify the terminal output #13

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

Open
dh-nunes wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Beautify the terminal output #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,6 @@ user-hooks = []
libc = "0.2.43"
lazy_static = "1.1"
semver = "0.9.0"

[build-dependencies]
colored = "1"
35 changes: 33 additions & 2 deletions build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
extern crate colored;
use colored::*;
use fs::File;
use io::{BufRead, Read, Write};
use path::{Path, PathBuf};
Expand Down Expand Up @@ -103,7 +105,7 @@ fn hook_already_exists(hook: &Path) -> bool {
fn write_script<W: io::Write>(w: &mut W) -> Result<()> {
macro_rules! raw_cmd {
($c:expr) => {
concat!("\necho '+", $c, "'\n", $c)
concat!("\nrun ", $c)
};
}

Expand Down Expand Up @@ -153,13 +155,42 @@ fn write_script<W: io::Write>(w: &mut W) -> Result<()> {
# Output at {}
#

set -e
animation() {{
sp='/-\|'
printf ' '
sleep 0.1
while true; do
printf '\b%.1s' "$sp"
sp=${{sp#?}}${{sp%???}}
sleep 0.1
done
}}
Copy link
Copy Markdown
Owner

@rhysd rhysd Nov 3, 2019

Choose a reason for hiding this comment

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

I don't think animation is helpful for users since users since users already know what script is being run. And animation may cause some junks on some terminals. I want to choose keeping generated script simple here.

Copy link
Copy Markdown
Author

@dh-nunes dh-nunes Nov 3, 2019

Choose a reason for hiding this comment

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

I would argue the animation is essential since it shows the process isn't hung - some of those commands can take a looong time to finish. The actual animation doesn't really matter I just added the simplest I could find. As for the terminals, it should all be posix compliant which should make it fine with linux terminals and I've tested on windows with Git Bash, Powershell and good old cmd and they all show it just fine. Still, keyword is 'should'.

Copy link
Copy Markdown
Owner

@rhysd rhysd Nov 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would argue the animation is essential since it shows the process isn't hung

Good point which I didn't care. I'm not understanding why user can know the process is not hung here. For example, let's say cargo test run by hook script hangs. Then the spinner will keep showing the animation. Rather than capturing output for spin animation, showing the log directly looks better way for me. cargo test shows output for each test case execution. So user can know test is still going ahead or is stuck at some test case. I think original husky does not hide output from underlying commands execution for check.

Copy link
Copy Markdown
Author

@dh-nunes dh-nunes Nov 6, 2019

Choose a reason for hiding this comment

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

Ohh right, for me showing everything just looked too cluttered :P Is there anything you want to keep from this pr?


run() {{
printf "%s" "{} '$*' "
animation &
animation_pid="$!"
output="$("$@" 2>&1)"
ret=$?
kill "$animation_pid"
wait "$animation_pid" 2>/dev/null
if [ $ret -eq 0 ]; then
printf "\b- {}\n"
else
printf "\b- {}\n"
printf "%s" "$output"
exit $ret
fi
}}
{}"#,
env!("CARGO_PKG_VERSION"),
env!("CARGO_PKG_HOMEPAGE"),
env!("CARGO_MANIFEST_DIR"),
path::MAIN_SEPARATOR,
env::var("OUT_DIR").unwrap_or_else(|_| "".to_string()),
"Running".bold(),
"Ok".bold().green(),
"Failed".bold().red(),
script
)?;
Ok(())
Expand Down
30 changes: 21 additions & 9 deletions test/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,10 @@ fn default_behavior() {
.unwrap()
.contains(format!("set by cargo-husky v{}", env!("CARGO_PKG_VERSION")).as_str()));
assert_eq!(
script.lines().filter(|l| *l == "cargo test --all").count(),
script
.lines()
.filter(|l| *l == "run cargo test --all")
.count(),
1
);
assert!(script.lines().all(|l| !l.contains("cargo clippy")));
Expand Down Expand Up @@ -186,19 +189,22 @@ fn change_features() {
assert_eq!(get_hook_script(&root, "pre-push"), None);

let script = get_hook_script(&root, "pre-commit").unwrap();
assert!(script.lines().all(|l| l != "cargo test"));
assert!(script.lines().all(|l| l != "run cargo test"));
assert_eq!(
script
.lines()
.filter(|l| *l == "cargo clippy -- -D warnings")
.filter(|l| *l == "run cargo clippy -- -D warnings")
.count(),
1
);
assert_eq!(script.lines().filter(|l| *l == "cargo check").count(), 1);
assert_eq!(
script.lines().filter(|l| *l == "run cargo check").count(),
1
);
assert_eq!(
script
.lines()
.filter(|l| *l == "cargo fmt -- --check")
.filter(|l| *l == "run cargo fmt -- --check")
.count(),
1
);
Expand All @@ -218,24 +224,30 @@ fn change_features_using_run_for_all() {

let script = get_hook_script(&root, "pre-commit").unwrap();
assert_eq!(
script.lines().filter(|l| *l == "cargo test --all").count(),
script
.lines()
.filter(|l| *l == "run cargo test --all")
.count(),
1
);
assert_eq!(
script
.lines()
.filter(|l| *l == "cargo clippy --all -- -D warnings")
.filter(|l| *l == "run cargo clippy --all -- -D warnings")
.count(),
1
);
assert_eq!(
script.lines().filter(|l| *l == "cargo check --all").count(),
script
.lines()
.filter(|l| *l == "run cargo check --all")
.count(),
1
);
assert_eq!(
script
.lines()
.filter(|l| *l == "cargo fmt --all -- --check")
.filter(|l| *l == "run cargo fmt --all -- --check")
.count(),
1
);
Expand Down