-
Notifications
You must be signed in to change notification settings - Fork 327
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 skip_code_autolink_to option #1759
Add skip_code_autolink_to option #1759
Conversation
@@ -559,6 +559,14 @@ defmodule ExDoc.Language.Elixir do | |||
end | |||
|
|||
defp url(string, mode, config) do | |||
if Enum.any?(config.skip_code_autolink_to, &(&1 == string)) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we allow regexes here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add if necessary later on? Unless you have an immediate need for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deprecated a module, so I'd like to do ~r/MyMod(\..*)?/
basically instead of having to list the references to the module alone and then to all the functions I mention. Unless I'm reading the feature wrong 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this would ever come up but the above regex will also catch MyNamespace.MyMod
. So maybe it should be: ~r/^MyMod(\..*)?/
. but then we'd still warn on callbacks and types so I suppose ~r/^([bt]:)?MyMod(\..*)?/
.
I think we can add the regex support but it needs to be used carefully. but yeah I'd rather have users spell out things explicitly to keep the code explicit too. how annoying is this gonna be for you? is this for a changelog? If so consider just cutting off old changelog entries, e.g. write:
The CHANGELOG for v1.15 and previous releases can be found [in the v1.15] branch
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we just make it a function and allow you to pass anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, +1 from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge this and we can make both skip_
options accept a function too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me.
I'm not even sorry for sounding like a broken radio, we really need to opt-in to auto linking by writing [`Foo.bar`]
(#1663), not opt-out.
@whatyouhide fyi: I do the above explicitly in my extras. It is annoying to type because I have to maintain links manually, e.g. in my README.md I have:
* Follows redirects (via [`redirect`] step.)
(...)
[`redirect`]: https://hexdocs.pm/req/Req.Steps.html#redirect/1
on the flip side, my not-so-auto links work on both https://hexdocs.pm/req/readme.html AND https://github.com/wojtekmach/req/blob/main/README.md.
so if you'd adopt it, the need for this PR disappears. But ngl, it's annoying to manually keep them, so +1 for the PR.
💚 💙 💜 💛 ❤️ |
Adds a new option
:skip_code_autolink_to
that given a list of strings, skips term autolinking if it matches one of the strings.I think
Enum.any?/2
will do fine, but if needed we can convert the list of strings given by the user to a MapSet and use it internally as well.Closes #1750