search

YiNNx / cmd-wrapped

👩‍💻 A CLI Tool for Command Line Insights
https://crates.io/crates/cmd-wrapped
MIT License
1.01k stars 27 forks source link

fix: empty command (#7) #8

Closed luckasRanarison closed 9 months ago

luckasRanarison commented 10 months ago

Closes #7.

YiNNx commented 9 months ago

Thanks for the fix! I’ll check it later

luckasRanarison commented 9 months ago

Oh, wait I think changing the Regex would be a better solution

Update: Why did you capture the newline in (?:[^#\n]|\n)? This could also cause an empty command.

static ref RE_BASH_HISTORY: Regex = Regex::new(r"(\d+)\n((?:[^#\n]|\n)+)").unwrap();
YiNNx commented 9 months ago

Maybe another approach is to refactor the Command::parse_line() function rather than modifying the regex parser's result? The Command::parse() function should be modified to verify if the line of command is empty or contains a valid command, such as combined commands like echo something &&

YiNNx commented 9 months ago

🤔 the new regex expression works for the empty line, too. But the Command::parse_line() function should also be fixed to handle the empty command in the filled line of combined commands or lines that are filled but not contain an invalid command.

YiNNx commented 9 months ago

Update: Why did you capture the newline in (?:[^#\n]|\n)? This could also cause an empty command.

I'm really sorry, but I'm struggling with my finals right now and might not be able to get back to you until Thursday :(

luckasRanarison commented 9 months ago

I'm really sorry, but I'm struggling with my finals right now and might not be able to get back to you until Thursday :(

There's no hurry :)

By the way, could I try adding atuin support if you haven't started yet and if you want...

YiNNx commented 9 months ago

By the way, could I try adding atuin support if you haven't started yet and if you want..

Absolutely! I haven't started yet and your contribution would be much appreciated if you're interested. 🙌

YiNNx commented 9 months ago

I added some code to improve error handling and condition restrictions for Command::parse_line(), which also help to avoid empty command stats. Now everything looks good to me! Thanks for the fix and sorry for being a little bit late to review it :(

YiNNx commented 9 months ago

Update: Why did you capture the newline in (?:[^#\n]|\n)? This could also cause an empty command.

This wounldn't cause the empty command stats, but it is indeed implemented not very elegantly :thinking: I will submit a PR later to fix it.