Implement ls #130
Implement ls #130
Conversation
There was a problem hiding this comment.
Do you intend to finish the last missing item in the check list? No hurries! The work here is amazing as is, and it's perfectly fine to merge without a complete implementation. 😃
Also I would like to keep track of the open PRs, because sometimes the author forgets to finish the draft, rendering us unable to merge the changes if finished! (I did this once 😆)
In this review I just added 2 comments, one is a nitpick, another one is that I did a breaking change in the coreutils_core API
ls/src/file.rs
Outdated
|
|
||
| use ansi_term::Color; | ||
|
|
||
| extern crate chrono; |
There was a problem hiding this comment.
You don't need extern crate on 2018 edition
ls/src/file.rs
Outdated
| /// Retrieves the file's user name as a string. If the `-n` flag is set, | ||
| /// the the user's ID is returned | ||
| pub fn user(&self) -> Result<BString, PasswdError> { | ||
| if self.flags.numeric_uid_gid { | ||
| return Ok(BString::from(self.metadata.uid().to_string())); | ||
| } | ||
|
|
||
| match Passwd::from_uid(self.metadata.uid()) { | ||
| Ok(passwd) => Ok(passwd.name().to_owned()), | ||
| Err(err) => Err(err), | ||
| } | ||
| } | ||
|
|
||
| /// Retrieves the file's group name as a string. If the `-n` flag is set, | ||
| /// the the group's ID is returned | ||
| pub fn group(&self) -> Result<BString, GroupError> { | ||
| if self.flags.numeric_uid_gid { | ||
| return Ok(BString::from(self.metadata.gid().to_string())); | ||
| } | ||
|
|
||
| match Group::from_gid(self.metadata.gid()) { | ||
| Ok(group) => Ok(group.name().to_owned()), | ||
| Err(err) => Err(err), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
In a recent commit I changed the Group and Passwd to return io::Result, because all os-module-like public API seems to return io::Error on problems, and most of the rest of the Core OS API was already returning a io::Error, so it made sense to change for better consistency with the Core API and with the Rust ecosystem.
Can you change that? I also can change that before merging, I have no problems with that
My apologies! I have been sitting on 03f3ab5 since December 17th (forgot to push this commit). Work and the holidays had me distracted. I will take care of your reviews, see if there is any clean up I can do, and move this PR out of a draft state. |
|
No need to apologize! We need to rest a little bit in the Holidays, I wasn't expecting a reply this month because of the Holidays LOL Take your time, no need to hurry! |
|
I am unsure when this began exactly, but I noticed today that my I would also like to note that the |
I think the reason that is happening is because the If I'm correct, I think a more clean solution is to only |
ls/src/output.rs
Outdated
|
|
||
| /// Writes the provided files in the default format. | ||
| pub(crate) fn default<W: Write>(files: Files, writer: &mut W, flags: Flags) -> io::Result<()> { | ||
| if !is_tty(&io::stdout()) { |
There was a problem hiding this comment.
Are we checking for the right thing here? In my mind I was thinking that here we wanted to check if the underlying W in BufWriter<W> is not a TTY, not specifically the stdout.
Let me know what you think, I may have misunderstood this part of the code
There was a problem hiding this comment.
I may be misunderstanding, but I believe it would be the same either way since the BufWriter instance is loaded with the stdout file descriptor.I went ahead and made this change though. I had to refactor to writer parameters to use BufWriter<Stdout> as it's type in order to get the file descriptor reference. Although I cannot remember why I was using the parameterized type for the writer to begin with.
|
LGTM! bors r+ |
|
bors r+ |
This PR implements the
lsutility. Below lists the arguments with the short and long flags.-A|--almost-all-C|--order-top-to-bottom-F|--classify-H|--no-dereference-L|--dereference-R|--recursive-S|--sort-size-a|--all-c|--file-status-modification-d|--directory-f|--no-sort-g|--no-owner-i|--inode-k|--block-size-l|--list-m|--comma-separate-n|--numeric-uid-gid-o|--no-group-p|--indicator-q|--hide-control-chars-r|--reverse-s|--size-t|--time-u|--last-accessed-x|--order-left-to-write-1|--one-per-line