-
Notifications
You must be signed in to change notification settings - Fork 2
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
Lingo Clean #63
base: main
Are you sure you want to change the base?
Lingo Clean #63
Conversation
9ff81db
to
cffe8e4
Compare
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.
I think this looks good, some questions only!
src/main.rs
Outdated
@@ -87,6 +89,11 @@ fn do_clone_and_checkout( | |||
Ok(git_rev) | |||
} | |||
|
|||
fn do_remove_folder(path: &str) -> Result<(), RemoveFolderError> { | |||
fs::remove_dir_all(path) | |||
.map_err(|_| RemoveFolderError(format!("Could not delete folder: {}", path))) |
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.
Does this yield an error if the build folder is not present? Should probably not do that?
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.
that is indeed a bug, fixed ist last commit
CommandSpec::Clean => { | ||
let output_root = &config.apps[0].output_root; | ||
if let Err(e) = remove_dir_all(&output_root.display().to_string()) { | ||
error!("lingo was unable to delete build folder! {e}"); |
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.
More or less the same error message is produced by remove_dir_all
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.
yes remove_dir_all
produces and error and here we handle it.
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.
But it seems like the error is: "lingo was unable to delete build folder! Could not delete folder: ..." which seems a little redundant
@@ -54,6 +55,12 @@ pub fn execute_command<'a>( | |||
} | |||
} | |||
} | |||
CommandSpec::Clean => { |
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.
Should we also remove Lingo.toml
here?
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.
no
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.
In my Makefile, make clean also removes Lingo.lock. If I don’t remove this file, then lingo build isn’t much use because it just checks out same previous version of the library repo that I’m trying to update. Is there a separate mechanism in Lingo for updating a library?
What if I want to update one library and not another? Removing Lingo.lock won’t do that.
Also, there are warning that should be dealt with.
We should probably stay inspired by
Which warning? Is it #60 ? |
The purpose of the file If you remove the |
No description provided.