Skip to content
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

Improve date format validation to include all supported cases #532

Closed
peter-joo opened this issue Jun 27, 2021 · 10 comments
Closed

Improve date format validation to include all supported cases #532

peter-joo opened this issue Jun 27, 2021 · 10 comments
Labels
good first issue Good for newcomers kind/bug Something isn't working

Comments

@peter-joo
Copy link

  • OS: Linux 5.12.9-1-MANJARO x86_64 GNU/Linux
  • lsd --version: lsd 0.20.1
  • echo $TERM: xterm-256color
  • echo $LS_COLORS: rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=00:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:.tar=01;31:.tgz=01;31:.arc=01;31:.arj=01;31:.taz=01;31:.lha=01;31:.lz4=01;31:.lzh=01;31:.lzma=01;31:.tlz=01;31:.txz=01;31:.tzo=01;31:.t7z=01;31:.zip=01;31:.z=01;31:.dz=01;31:.gz=01;31:.lrz=01;31:.lz=01;31:.lzo=01;31:.xz=01;31:.zst=01;31:.tzst=01;31:.bz2=01;31:.bz=01;31:.tbz=01;31:.tbz2=01;31:.tz=01;31:.deb=01;31:.rpm=01;31:.jar=01;31:.war=01;31:.ear=01;31:.sar=01;31:.rar=01;31:.alz=01;31:.ace=01;31:.zoo=01;31:.cpio=01;31:.7z=01;31:.rz=01;31:.cab=01;31:.wim=01;31:.swm=01;31:.dwm=01;31:.esd=01;31:.jpg=01;35:.jpeg=01;35:.mjpg=01;35:.mjpeg=01;35:.gif=01;35:.bmp=01;35:.pbm=01;35:.pgm=01;35:.ppm=01;35:.tga=01;35:.xbm=01;35:.xpm=01;35:.tif=01;35:.tiff=01;35:.png=01;35:.svg=01;35:.svgz=01;35:.mng=01;35:.pcx=01;35:.mov=01;35:.mpg=01;35:.mpeg=01;35:.m2v=01;35:.mkv=01;35:.webm=01;35:.webp=01;35:.ogm=01;35:.mp4=01;35:.m4v=01;35:.mp4v=01;35:.vob=01;35:.qt=01;35:.nuv=01;35:.wmv=01;35:.asf=01;35:.rm=01;35:.rmvb=01;35:.flc=01;35:.avi=01;35:.fli=01;35:.flv=01;35:.gl=01;35:.dl=01;35:.xcf=01;35:.xwd=01;35:.yuv=01;35:.cgm=01;35:.emf=01;35:.ogv=01;35:.ogx=01;35:.aac=00;36:.au=00;36:.flac=00;36:.m4a=00;36:.mid=00;36:.midi=00;36:.mka=00;36:.mp3=00;36:.mpc=00;36:.ogg=00;36:.ra=00;36:.wav=00;36:.oga=00;36:.opus=00;36:.spx=00;36:.xspf=00;36:

Expected behavior:

lsd shall display the nanoseconds like the date command does.
date command:
date '+%Y.%m.%d-%H.%M.%S.%N'
date command's output:
2021.06.27-16.00.33.013881009

Actual behavior

lsd command:
lsd --date '+%Y.%m.%d-%H.%M.%S.%N'
lsd ommand's output:
error: Invalid value for '--date <date>...': invalid format specifier: %N

@peter-joo
Copy link
Author

The TIME_STYLE way also fails:
TIME_STYLE='+%Y.%m.%d-%H.%M.%S.%N' lsd

output:
lsd: Not a valid date format: +%Y.%m.%d-%H.%M.%S.%N.

@meain
Copy link
Member

meain commented Jun 27, 2021

There is some subtle differences in the date format specifiers in the lib (chrono) that we are using to do that. In this case, it would be %f instead of %N.

TIME_STYLE='+%Y.%m.%d-%H.%M.%S.%f' lsd

You can find the full list here: https://docs.rs/chrono/0.4.19/chrono/format/strftime/index.html#specifiers

@peter-joo
Copy link
Author

That's a very good, correct and fast response, thank you for that :)

However the formatting part for %f does not work:
lsd --date='+%Y.%m.%d-%H.%M.%S.%3f'
output:
error: Invalid value for '--date <date>...': invalid format specifier: %3

Consider including those to show millisecond precision instead of nanosecond one:
https://docs.rs/chrono/0.4.19/chrono/format/strftime/index.html#specifiers

%f, %.f, %.3f, %.6f, %.9f, %3f, %6f, %9f:
The default %f is right-aligned and always zero-padded to 9 digits for the compatibility with glibc and others, so it always counts the number of nanoseconds since the last whole second. E.g. 7ms after the last second will print 007000000, and parsing 7000000 will yield the same.

The variant %.f is left-aligned and print 0, 3, 6 or 9 fractional digits according to the precision. E.g. 70ms after the last second under %.f will print .070 (note: not .07), and parsing .07, .070000 etc. will yield the same. Note that they can print or read nothing if the fractional part is zero or the next character is not ..

The variant %.3f, %.6f and %.9f are left-aligned and print 3, 6 or 9 fractional digits according to the number preceding f. E.g. 70ms after the last second under %.3f will print .070 (note: not .07), and parsing .07, .070000 etc. will yield the same. Note that they can read nothing if the fractional part is zero or the next character is not . however will print with the specified length.

The variant %3f, %6f and %9f are left-aligned and print 3, 6 or 9 fractional digits according to the number preceding f, but without the leading dot. E.g. 70ms after the last second under %3f will print 070 (note: not 07), and parsing 07, 070000 etc. will yield the same. Note that they can read nothing if the fractional part is zero.

@meain
Copy link
Member

meain commented Jun 27, 2021

Ooh, this looks like a bug. We are doing an extra validation check when we take input here. Looks like right now it is only capable of parsing a subset of the spec.

@meain meain added good first issue Good for newcomers kind/bug Something isn't working labels Jun 27, 2021
@peter-joo
Copy link
Author

Great, thanks for the confirmation!

What am I supposed to do with this ticket? Rename the title? Enter an other one? Leave it like this?

@zwpaper
Copy link
Member

zwpaper commented Jun 28, 2021

how about making this even further by adding the --time-style flag like GNU ls and also updating the format

@meain
Copy link
Member

meain commented Jun 28, 2021

@zwpaper I guess we could add --time-style flag as an alias for --date. I am guessing it does not do anything more than that. About fixing the format, I don think it would be worth it. We will have to change from using chrono as the datetime lib and that might be a non trivial amount of change for this.

@peter-joo I think I will change the title to something a bit better.

@meain meain changed the title --date option fails when used with +date-time-format which contains %N = nanoseconds Improve date format validation to include all supported cases Jun 28, 2021
@zwpaper
Copy link
Member

zwpaper commented Jun 30, 2021

@meain as we always do, we were trying to keep the compatibility with GNU ls, but this breaks it.

one tricky solution could be we figure out the difference between chrono and datetime lib, then replace the flag before sending it to the lib.

In this way, there might only be a minor change and keep the compatibility.

But it is really tricky, and we could try to improve it in a long-term way.

meain pushed a commit that referenced this issue Oct 14, 2021
…Validation extended to support mentioned case.
meain pushed a commit that referenced this issue Oct 14, 2021
meain pushed a commit that referenced this issue Oct 14, 2021
meain pushed a commit that referenced this issue Oct 14, 2021
meain pushed a commit that referenced this issue Oct 14, 2021
@meain
Copy link
Member

meain commented Oct 14, 2021

Thanks to @arkadiuszbielewicz , we now support all the the possible options in chrono as of #567

@peter-joo
Copy link
Author

peter-joo commented Aug 20, 2022

it works, thank you :)

lsd --date='+%Y.%m.%d-%H.%M.%S.%3f'
.rw-r--r-- pj MANJARO 2022.08.20-14.02.01.069 45207 junk.dat

You may want to close this issue.

@zwpaper zwpaper closed this as completed Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants