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: Added auto, always, never modifiers to --hyperlink argument (V2) #1059

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

Conversation

nikelborm
Copy link

@nikelborm nikelborm commented Jul 16, 2024

This is an ideological continuation of @mrnossiom's work in #746 (as I've learned already after I started coding the current PR myself) with added shell completions that don't have merge conflicts with the main branch.

Description

Add a WHEN modifier to the --hyperlink argument. This matches the ls behavior and the --color argument behavior.

I think ls is doing it right because you might want to enable --hyperlink in a shell alias but still get plain text when piping into some other program without control chars in the way. Setting --hyperlink in a shell alias is useful for terminal integration (e.g. Kitty).

The public API is compatible since --hyperlink defaults to auto and doesn't break. However, it changes the behavior, with no value the option worked like always but now defaults to auto. Even though I consider auto the right default, maybe it could be changed to always to keep the old behavior.

@mrnossiom
Copy link
Contributor

I can merge shell completions updates into my branch, but I don't see how this PR resolves what was blocking on mine.

@nikelborm
Copy link
Author

nikelborm commented Jul 16, 2024

I can merge shell completions updates into my branch, but I don't see how this PR resolves what was blocking on mine.

When I started doing it, I started doing it myself following already established patterns in the current project. But I started doing it on top of the latest main branch. Then I found your PR that looked similar and merged what i done with what you did and mentioned you in the commits where our changes looked similar. I didn't really tested what kind of conflicts exist on your branch, I just looked on github's banner
Screenshot from 2024-07-16 14-41-49
And since I built this PR on top of latest commit in main branch it doesn't yet have any conflicts.

@mrnossiom
Copy link
Contributor

For conflicts, you have rebasing. But that's not blocking. The issue we had was that hyperlink OSCs were not taken into account when calculating width and that end up messing with the --grid/G flag.

I just rebased and force-pushed on my end. Works fine apparently.

gierens
gierens previously approved these changes Jan 19, 2025
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.

Code and functionality on my system looks good ... Gotta check this failing BSD test and conflict with main, though.

@gierens
Copy link
Member

gierens commented Jan 20, 2025

Ok, I rebased main into this branch and resolved the conflicts.

@gierens
Copy link
Member

gierens commented Jan 20, 2025

I rebased main again which added a fix for the FreeBSD CI, should run through now ... also ran a short benchmark, doesn't appear to introduce any performance penalty, so again LGTM, thanks.

@gierens gierens self-requested a review January 20, 2025 17:20
gierens
gierens previously approved these changes Jan 20, 2025
MartinFillon
MartinFillon previously approved these changes Jan 23, 2025
@nikelborm nikelborm force-pushed the main branch 2 times, most recently from 21e5316 to bc3edc2 Compare January 24, 2025 00:48
Freed-Wu and others added 10 commits February 14, 2025 11:21
… can

Co-authored-by: Milo Moisson <milomoisson@gmail.com>
Co-authored-by: Sandro-Alessio Gierens <sandro@gierens.de>
Co-authored-by: Milo Moisson <milomoisson@gmail.com>
Co-authored-by: Sandro-Alessio Gierens <sandro@gierens.de>
Co-authored-by: Sandro-Alessio Gierens <sandro@gierens.de>
Co-authored-by: Milo Moisson <milomoisson@gmail.com>
Co-authored-by: Sandro-Alessio Gierens <sandro@gierens.de>
@cafkafk cafkafk dismissed stale reviews from gierens and MartinFillon February 20, 2025 05:32

The merge-base changed after approval.

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.

5 participants