Skip to content

printer: deduplicate hyperlink alias names#3103

Closed
ltrzesniewski wants to merge 1 commit intoBurntSushi:masterfrom
ltrzesniewski:hyperlink-aliases
Closed

printer: deduplicate hyperlink alias names#3103
ltrzesniewski wants to merge 1 commit intoBurntSushi:masterfrom
ltrzesniewski:hyperlink-aliases

Conversation

@ltrzesniewski
Copy link
Contributor

This refactors the way hyperlink aliases are handled by embedding the code in a HyperlinkAlias type.

Currently, this removes a duplicate alias list from the HyperlinkFormat flag in defs.rs, but it should also provide the required features to avoid adding additional duplicates in the following PRs:

/cc @ilyagr @okdana

@ltrzesniewski

This comment was marked as resolved.

This refactors the way hyperlink aliases are handled by embedding the code in a HyperlinkAlias type.

Currently, this also removes a duplicate alias list from the HyperlinkFormat flag in defs.rs, but it is expected to later be used in other features such as shell completion scripts.
BurntSushi pushed a commit that referenced this pull request Sep 6, 2025
This exports a new `HyperlinkAlias` type in the `grep-printer` crate.
This includes a "display priority" with each alias and a function for
getting all supported aliases from the crate.

This should hopefully make it possible for downstream users of this
crate to include a list of supported aliases in the documentation.

Closes #3103
@BurntSushi BurntSushi added the rollup A PR that has been merged with many others in a rollup. label Sep 6, 2025
@BurntSushi
Copy link
Owner

Thank you for adding this! I ended up re-working this a bit to make the API a little less convenient, but in exchange, it should hopefully be easier to evolve in semver compatible ways if necessary. That is, the revised API doesn't encode as many assumptions.

I was somewhat on the fence about accepting this. I'm not really a huge fan of this much ceremony just to avoid a little duplication. In particular, there are other ways to ensure things don't get out of sync, i.e., a CI test or something. But I think overall this is a pretty small and reasonable change, and it might be useful to consumers of grep-printer to be able to see the list of supported aliases in a programmatic fashion.

@ltrzesniewski
Copy link
Contributor Author

ltrzesniewski commented Sep 7, 2025

Thanks for accepting this and using it in the other PRs! I thought the grep-printer crate was internal to ripgrep, so its API could be flexible. I'll keep this in mind.

Just one nitpick on display_priority: I think I'd rather use a signed type, so some aliases could eventually be forced to be at the end of the list. Maybe Option wouldn't be needed in that case, as the default priority could be 0.

@BurntSushi
Copy link
Owner

BurntSushi commented Sep 7, 2025

Thanks for checking it out! A signed type is probably wiser. But users of the crate can't rely on any specific priority that is given for an alias and there is no way for users to create a new alias or assign a priority. So its meaning is entirely under the control of the implementation.

But yeah, I am very grumpy about ripgrep's crates being on crates.io. I kind of regret doing it because it invites so much ceremony. But there are a healthy number of dependents of the grep crate (which is just a facade for crates like grep-printer). Because it's all on crates.io, a semver boundary really does have to be respected unfortunately.

I am more lenient about breaking changes than I usually am for more fundamental "ecosystem" crates. But I do still try to avoid them when it isn't too much of a pain otherwise. I'm just used to it after years of doing it with other crates.

BurntSushi pushed a commit that referenced this pull request Sep 10, 2025
This exports a new `HyperlinkAlias` type in the `grep-printer` crate.
This includes a "display priority" with each alias and a function for
getting all supported aliases from the crate.

This should hopefully make it possible for downstream users of this
crate to include a list of supported aliases in the documentation.

Closes #3103
BurntSushi pushed a commit that referenced this pull request Sep 19, 2025
This exports a new `HyperlinkAlias` type in the `grep-printer` crate.
This includes a "display priority" with each alias and a function for
getting all supported aliases from the crate.

This should hopefully make it possible for downstream users of this
crate to include a list of supported aliases in the documentation.

Closes #3103
@BurntSushi
Copy link
Owner

I changed the type of priority from Option<u16> to Option<i16>.

The Option could probably be dropped, but I left it there. I think either option is workable.

BurntSushi pushed a commit that referenced this pull request Sep 20, 2025
This exports a new `HyperlinkAlias` type in the `grep-printer` crate.
This includes a "display priority" with each alias and a function for
getting all supported aliases from the crate.

This should hopefully make it possible for downstream users of this
crate to include a list of supported aliases in the documentation.

Closes #3103
@ltrzesniewski ltrzesniewski deleted the hyperlink-aliases branch September 20, 2025 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rollup A PR that has been merged with many others in a rollup.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants