8000 ls: add support for +FORMAT in timestyle by dmatos2012 · Pull Request #3988 · uutils/coreutils · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ls: add support for +FORMAT in timestyle #3988

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 5, 2022
Merged

ls: add support for +FORMAT in timestyle #3988

merged 1 commit into from
Oct 5, 2022

Conversation

dmatos2012
Copy link
Contributor

Hello,
This PR addresses issue #3625, which adds support to the +FORMAT date in --time-style. Given the dynamic argument, I created a custom parsing function to accommodate for the new format.

I have added the corresponding tests, but probably my code added might not be up to standards, as I am new at it and trying to improve, and basically doing what the borrow checker tells me every time :). I would be more than happy to make changes to improve it.

I did notice using the +Format on the linked issue, namely $ TIME_STYLE="+%Y-%m-%d %H:%M:%S.%N %z" ls exe -al breaks with

thread 'main' panicked at 'a Display implementation returned an error unexpectedly: Error', /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/alloc/src/string.rs:2406:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Basically, it fails by providing time.format("%Y-%m-%d %H:%M:%S.%N %z" ), but it looks more on the crate used.

Thanks for guidance and potential feedback :)

@tertsdiepraam tertsdiepraam linked an issue Sep 29, 2022 that may be closed by this pull request
Implement TIME_STYLE="+DATE" ls #3625
Closed
@tertsdiepraam
Copy link
Member
tertsdiepraam commented Sep 29, 2022 •
edited
Loading

%N seems to be part that causes the panic. I'll try to make a reproducible case for the time chrono crate.

Edit: %N is a GNU extension to strftime (https://github.com/coreutils/gnulib/blob/master/lib/nstrftime.c#L1115) and criminally underdocumented. So it makes sense that chrono doesn't support it and if we eventually want to support it, we'd need to implement it ourselves. It is a problem that chrono panics instead of returning a nice error though.

@dmatos2012
Copy link
Contributor Author
dmatos2012 commented Sep 29, 2022 •
edited
Loading

I agree on that it should not panic, however depending on the use, it would seem that the %N might not be a problem, but I am not very sure whether it would be worth to implement it. I can perhaps give it a try in a different PR, but maybe my efforts can be used on some other issues on this repo.

Btw: Pushed again to fix the cspell failing test.

@tertsdiepraam
Copy link
Member

Oh yeah, %N is definitely something to figure out later. No need to worry about it :)

@sylvestre sylvestre merged commit ae7c45d into uutils:main Oct 5, 2022
@dmatos2012 dmatos2012 deleted the add-date-format-ls branch October 24, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement TIME_STYLE="+DATE" ls
3 participants
@dmatos2012 @tertsdiepraam @sylvestre
0