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

Add plugin to skip command linker #153

Merged
merged 2 commits into from
Dec 19, 2022
Merged

Add plugin to skip command linker #153

merged 2 commits into from
Dec 19, 2022

Conversation

yathomasi
Copy link
Contributor

By default, our command line linkers always link any inline code based on dvc, cml, ... our tools.

If we want to customize them we can always wrap the inline code with a link eg:

[`dvc ...`](link)

This PR adds a plugin to allow us to have no link at all. For that, we can pass no value. eg:

[`dvc ...`]()

closes iterative/cml.dev#424

@yathomasi yathomasi requested a review from a team as a code owner December 13, 2022 12:46
@yathomasi yathomasi self-assigned this Dec 13, 2022
@yathomasi yathomasi requested a review from casperdcl December 13, 2022 12:47
@rogermparent rogermparent temporarily deployed to gatsby-theme-null-link--yl03qi December 13, 2022 12:49 Inactive
Copy link
Contributor

@rogermparent rogermparent left a comment

Choose a reason for hiding this comment

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

I don't really love the solution of adding a remark plugin with new syntax to override the auto-linker instead of constraining what the auto-linker operates on, but if @iterative/docs feels differently I won't stand in the way. If nothing else, it could be a decent mid-ground and it won't be very difficult to backport empty links after adjusting autolinker behavior.

@yathomasi
Copy link
Contributor Author

constraining what the auto-linker operates on

We have a constraint on linking when there is inline code with cml|dvc|...tools and their subcommand exists or if there is an option we do check if they exist. So, I don't see how we can constrain it more from the auto-linker side.

Also, we already use links to override the autogenerated links. So, passing an empty value to ignore auto-link seemed a good approach to me.

@yathomasi yathomasi requested a review from a team December 14, 2022 06:45
@rogermparent
Copy link
Contributor

rogermparent commented Dec 14, 2022

So, I don't see how we can constrain it more from the auto-linker side.

As far as I understand, the originally suggested constraint when this was proposed is

cml thingy --flag correctly links to /doc/ref/thingy#--flag but cml thingy --flag --another-flag really shouldn't IMO.

It seems to me like auto-linking only code blocks with a single flag would be the most expected, unless there is a good reason to auto-link commands with more than one flag instead of manually linking those cases (@iterative/docs may be able to think of an example). Using [link]() feels like it should be more of a last resort on something that would otherwise definitely be linked, and I also don't see a lot of that use case if we implement the former behavior.

This feature makes sense, at least in the current context, but even then saying that all multi-flag code examples should use it feels a bit too intrusive.

@yathomasi
Copy link
Contributor Author

yathomasi commented Dec 19, 2022

I could only find a similar case scenario in dvc.org

If we want to fully ignore the command line linker in a similar case scenario then feel free to open a ticket and continue with the discussion.
Personally, I would allow the linker as these are rare and the current solution [`skip command linker`]() would allow the skip if we want.

cc: @iterative/docs

@yathomasi yathomasi temporarily deployed to gatsby-theme-null-link--yl03qi December 19, 2022 15:04 Inactive
@yathomasi yathomasi merged commit eaa5ece into main Dec 19, 2022
@yathomasi yathomasi deleted the null-link-plugin branch December 19, 2022 15:23
@jorgeorpinel

This comment was marked as resolved.

@jorgeorpinel
Copy link

jorgeorpinel commented Dec 19, 2022

auto-linking only code blocks with a single flag would be the most expected, unless there is a good reason to auto-link commands with more than one flag

OK, I guess it's a separate Q from #156 (that one is more about whether the subcommands and flags actually exist). BTW, should we move it to this repo?

To answer the Q, I think a decent default behavior would be to link to the specific option only when there's a single flag indeed, but still link to the general ref. page if there are more (due to ambiguity). Other options:

  • Always link to the general page
  • Always link to #options if there are flags present

@yathomasi
Copy link
Contributor Author

BTW, should we move it to this repo?

Sure, we can move it here as core logic is here.

To answer the Q, I think a decent default behavior would be to link to the specific option only when there's a single flag indeed, but still link to the general ref. page if there are more (due to ambiguity). Other options:

  • Always link to the general page
  • Always link to #options if there are flags present

Yes, I agree with this and it works as mentioned except for multiple options. Right now, it links to the first option from the multiple options and ignores anything more. But, as mentioned such usage is rare for us.

@jorgeorpinel jorgeorpinel mentioned this pull request Dec 20, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to stop this auto-hyperlinking?
3 participants