-
Notifications
You must be signed in to change notification settings - Fork 41
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
Destination option for agama logs store #823
Conversation
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 would work a little on simplifying the parse_dest
function. Beware, I have not tested my suggestions, so take them with a grain of salt 😉. Perhaps you should write a small unit test...
rust/agama-cli/src/logs.rs
Outdated
let err = io::Error::new(io::ErrorKind::InvalidInput, "Invalid destination path"); | ||
|
||
match dest { | ||
None => Ok(default), |
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.
What if /tmp/agama_logs
is already a directory? What if tomorrow we decide to change the default value? I would run the same checks whether it is the default or a value from the user.
This way, we could drop the match
and save some nesting:
let mut buff = dest.unwrap_or(PathBuf::from(DEFAULT_RESULT));
let path = buff.as_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.
BTW, why not buffer
instead of buff
? We are just saving two letters :-)
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.
if /tmp/agama_logs
is already directory, then you will end up with /tmp/agama_logs.tar.bzip2
archive.
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.
regarding buff`` well ... just matter of habit as i have been using
buff```` and dest
since I was at school, so it is kind of convention for me ;-) It is even used in man pages of such tools like ```cp```, ```rsync```, e.g. ```rsync``` defines its usage as ```rsync [OPTION...] SRC... [DEST]``` even in some options like ```--copy-dest=DIR```.
However, I don't have problem to extend it. It definitely is not stopper for me ;-)
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.
Not even for me, I was curious especially about the "buff" case :-)
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.
About the /tmp/agama_logs
, my point is that I would run the same checks in both cases. And in that case, you could save the match
statement (too much nesting for my taste). Anyway, I am not going to block the approval. Please, fix the conflict in the changes file.
861234a
to
9066206
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 have clarified what I meant about the checks of /tmp/agama_logs
.
rust/agama-cli/src/logs.rs
Outdated
let err = io::Error::new(io::ErrorKind::InvalidInput, "Invalid destination path"); | ||
|
||
match dest { | ||
None => Ok(default), |
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.
About the /tmp/agama_logs
, my point is that I would run the same checks in both cases. And in that case, you could save the match
statement (too much nesting for my taste). Anyway, I am not going to block the approval. Please, fix the conflict in the changes file.
I got it ... I'm just testing it. Just used an opportunity and cleaned even another part. |
9b8bbf3
to
ea4b05e
Compare
Problem
When storing logs during installation it would be nice to be able to define destination path
Solution
added an option for
agama logs store
Testing
Screenshots
TODO: add one