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

feat!: support env EZA_QUOTING_STYLE, replace --no-quotes with --quotes=always,auto,never #587

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

PThorpe92
Copy link
Member

#584

Got me thinking about the different quoting styles that LS supports, and I think it's reasonable to offer some of this. Also, I am not a fan of the --no-x flags. I think it makes much more sense to offer an ENV var solution for NEVER and default it to auto and then offer a way to pass it in the command line. Idk who would want all their filenames quoted but hey, it's an option for LS.

It would obviously remain defaulted to auto, so the only thing that would break would be the --no-quotes flag would have to either be --quotes=never or the EZA_QUOTING_STYLE=never would have to be set.

Also: for anyone testing this, we are still waiting on #569 to get merged to fix the width/spacing issue

@PThorpe92 PThorpe92 requested a review from cafkafk as a code owner October 31, 2023 16:55
@PThorpe92 PThorpe92 changed the title feat!: support env EZA_QUOTING_STYLE, replace --no-quotes with --quotes=always,atuo,never feat!: support env EZA_QUOTING_STYLE, replace --no-quotes with --quotes=always,auto,never Oct 31, 2023
@PThorpe92
Copy link
Member Author

PThorpe92 commented Oct 31, 2023

EDIT: should be all set now

@PThorpe92 PThorpe92 marked this pull request as draft October 31, 2023 17:19
@PThorpe92 PThorpe92 marked this pull request as ready for review October 31, 2023 20:22
MartinFillon
MartinFillon previously approved these changes Nov 1, 2023
Copy link
Contributor

@MartinFillon MartinFillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great. Maybe we could use some tests ?

@cafkafk
Copy link
Member

cafkafk commented Nov 3, 2023

It would obviously remain defaulted to auto, so the only thing that would break would be the --no-quotes flag would have to either be --quotes=never or the EZA_QUOTING_STYLE=never would have to be set.

I mean, right now I do kinda feel comfortable with just saying goodbye to the --no-quotes flag, but post-clap post-1.0 we'd probably not do this, and have --no-quotes equivalent to --quotes=never.

@cafkafk
Copy link
Member

cafkafk commented Dec 28, 2024

SO, I tried to fix the conflicts here (sorry for necrobumping but I thought it would be fun to try and clear out a bit of backlog). Did we ever talk about perhaps keeping around a backward compat --no-quotes flag? We could hide it from the user (not put in readme etc), but just make it behave like --quotes=never to avoid making this a breaking change

@PThorpe92
Copy link
Member Author

I honestly don't remember if we did. Seems like a reasonable thing to do tho, just to avoid breaking anything

Copy link
Member

@gierens gierens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the --quotes options to the bash completion and missing automatic option to flags.rs WHEN array, as well as a few minor fixes here and there. Otherwise LGTM, would also prefer that to the current --no-quotes! Great!

PThorpe92 and others added 4 commits January 14, 2025 10:25
…support EZA_QUOTING_STYLE

Let's imagine this went well...

Signed-off-by: Preston Thorpe <[email protected]>
Co-authored-by: Preston Thorpe <[email protected]>
Co-authored-by: Christina Sørensen <[email protected]>
Co-authored-by: Sandro-Alessio Gierens <[email protected]>
Signed-off-by: Preston Thorpe <[email protected]>
Co-authored-by: Preston Thorpe <[email protected]>
Co-authored-by: Christina Sørensen <[email protected]>
Signed-off-by: Preston Thorpe <[email protected]>
Co-authored-by: Preston Thorpe <[email protected]>
Co-authored-by: Christina Sørensen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

4 participants