-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
ls
: implement device symbol and id
#2145
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
Conversation
Looks like the |
@tertsdiepraam yeah, would you mind opening a bug about this? |
Could you please fix
|
Seems like that other PR was not successful then :( I'll try again soon! |
Could you please fix the conflict? thanks |
Should be all good now! Only code cov failed due to an unrelated test. |
conflicting again with the recent pr :( sorry |
No problem! I was expecting that to happen :) |
let dev: u64 = metadata.rdev(); | ||
let major = (dev >> 8) as u8; | ||
let minor = dev as u8; | ||
return format!("{}, {}", major, minor); |
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.
is it expected that we don't have tests for this?
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.
Yeah, I wasn't sure how to test that. If you know a way to create (or mock?) devices in the test suite I'd be happy to add some tests!
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.
dunno, sorry
Closes #2140
This is a quick fix to show the correct symbols for character device and block device, fifo and pipe special files and show the minor and major id numbers of devices. I'm not sure how to test this, so any ideas on that are welcome!
The id numbers are not spaced correctly yet, because that would require a bit of a rewrite of the
long
format. I would rather do that in another PR (which should also fix #2139). This is what it looks like now:and here is what it should eventually look like: