-
Notifications
You must be signed in to change notification settings - Fork 23
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
Big refactor #57
Big refactor #57
Conversation
No idea why I can't select you in the reviewers dropdown, but @0xd34b33f, @filalex77 & @lberrymage, could you review this? |
@tversteeg Okay, this is big. I'll take a look later today. |
.map(|package| package.color_full_name()) | ||
// Make it a vector again | ||
.collect::<_>(); | ||
let package_names: Vec<&str> = package_names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needless collect. Just call map(|x| x.as_str())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I can't, because when I try to convert it to a &str
the original String
goes out of scope and gets deleted.
// First we split the line into separating characters | ||
let lines = line | ||
.split(|c| c == ';' || c == '|' || c == '&') | ||
// Then try to find the proper package manager for each line, this also filters out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice solution! And much faster, then regexes. But it can be a lot of tricky moments, a-la bash -c 'cargo install package'
. So maybe we need to use something like this. Or it's an overkill (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice solution! And much faster, then regexes. But it can be a lot of tricky moments, a-la
bash -c 'cargo install package'
. So maybe we need to use something like this. Or it's an overkill (:
Actually catching bash -c 'cargo install package'
isn't a bad idea right?
So maybe we need to use something like this. Or it's an overkill (:
I was considering this as well but I also want to support different shells..
info!("Commiting with message \"{}\".", commit_msg); | ||
git::add_file(&self.path, &*self.config.repo.file, false)?; | ||
git::commit_all(&self.path, &*commit_msg, false, false)?; | ||
println!("Commiting with message \"{}\".", commit_msg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why println, if there's info!
macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The info macro prints useless timestamps into the console.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easily tweakable using fern crate. We can leave info and create verbosity switcher to show messages with different priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I'll first merge this and implement that afterwards.
@all-contributors please add @0xd34b33f for review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so I spent a solid half hour with this PR, and I must say - your code is very readable and enjoyable. I appreciate all the comments. Nothing serious stood out, just a nitpick. I'd be happy to review more!
I'll be leaving a review later today; like filalex77 said, there's quite a bit to go through! |
@lberrymage no problem take your time, I still have to implement multi-argument sub-commands which I'll do tomorrow if I'll have time. After that I'll merge this and create an alpha release version so the changes can be used for a while. |
@all-contributors please add @filalex77 for review |
Changes:
src/package_manager/
. All you have to do to create a new one is copy another, change the values & add it tosrc/package_manager/mod.rs
..emplace
file now has a field for flags and a slightly different format..emplace
files are automatically migrated to the new version.PipUser
andPip3User
are not used anymore, they are handled byPip
&Pip3
with flags.RustupComponent
toRustup
.info!
which prints a timestamp.TODO for this PR:
Implement prefix for commands (Nix).TODO later: