Looks good! Thanks!

Let me know if you'd like me to add more tests

Could you add a test for --classify=none as well? auto is a bit complicated but it would be nice to check none.

Have added a test for the none case for --indicator-style, --ind and --classify with value none

Thank you!

The GNU tests seem to be failing for one of the ls tests which is due to my change.

I suspect its because in tests/ls/file-type.sh of the GNU test suite it tries to run

ls -F sub > out || fail=1

With my change now it tries to use sub as the value to the flag. I suppose the solution would be to enforce using = to separate the flag and the value ?

Okay, apparently that is not the fix. The GNU version works just fine with this command. Probably needs some different way of handling the flag argument with clap I guess.

My bad, seems like it the GNU ls does require an = to separate. Have made the change so hopefully it should pass this time. I did notice once inconsistency though. In the GNU version of ls, the short version of the --classify flag -F does not allow taking a value whereas my implementation allows that. Not sure if there is a flag in clap to change that. Couldn't find it when I looked. Is that okay ? Or do we want to fix that in some way ?

Have made the change so hopefully it should pass this time.

Nice! I didn't even think of that.

Not sure if there is a flag in clap to change that.

I don't think there is. Other utils work around this by creating a separate flag for the short flag, which is not great, so I'm fine with leaving it like this, but it'd be great if you could explain that inconsistency in a comment in the code.

Just an observation. The original post asks for never but the code is using none. Not sure if this is important but just wanted to point out.

Good catch! It should indeed be never

My bad, I noticed that difference when implementing but forgot to call it out. I used the GNU docs as my reference - https://www.gnu.org/software/coreutils/manual/html_node/General-output-formatting.html

‘-F’
‘--classify [=when]’
‘--indicator-style=classify’

    Append a character to each file name indicating the file type. Also, for regular files that are executable, append ‘*’. The file type indicators are ‘/’ for directories, ‘@’ for symbolic links, ‘|’ for FIFOs, ‘=’ for sockets, ‘>’ for doors, and nothing for regular files. when may be omitted, or one of:

        none - Do not classify. This is the default.
        auto - Only classify if standard output is a terminal.
        always - Always classify. 

    Specifying --classify and no when is equivalent to --classify=always. Do not follow symbolic links listed on the command line unless the --dereference-command-line (-H), --dereference (-L), or --dereference-command-line-symlink-to-dir options are specified.

Is there any reason we want it to be never instead of none ? I now see that never is used in many of the other options in addition to none.

Oh I see. It both works! Here's some GNU output if the option is wrong:

ls: invalid argument ‘sdfghferfg’ for ‘--classify’
Valid arguments are:
  - ‘always’, ‘yes’, ‘force’
  - ‘never’, ‘no’, ‘none’
  - ‘auto’, ‘tty’, ‘if-tty’
Try 'ls --help' for more information.

The docs are apparently inconsistent. never is mentioned in --help and man on my machine (coreutils 9.0):

  -F, --classify[=WHEN]      append indicator (one of */=>@|) to entries;
                               WHEN can be 'always' (default if omitted),
                               'auto', or 'never'

Ahh, I see. I should have just gone ahead and tested what the actual thing accepted. Should I modify the help information to also mention these additional values when I add them or should I let it be as it is ?

I think you can leave it like this, clap will also show all possible values and otherwise it will become a bit cluttered I think

Done ! :)

Seems like the windows build failed - But I doubt it is due to my change ?

Yeah, it's probably unrelated

Do we need to rerun that check so that it passes and PR can be reviews/merged ? Or is it a non-random failure that needs to be fixed in the pipeline elsewhere ?

Looks all good! Thanks!