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

Autolink directive #1264

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Autolink directive #1264

wants to merge 2 commits into from

Conversation

nobu
Copy link
Member

@nobu nobu commented Dec 31, 2024

To control cross-reference linking.
Fix #1254.

@@ -1378,4 +1378,15 @@ def self.load_options
options
end

def self.boolean(flag, message = nil)
if flag == true or flag == false
@autolink = flag
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@autolink = flag
flag

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@st0012
Copy link
Member

st0012 commented Dec 31, 2024

Can you add more examples in the description on how this would be used?

I think it'd be like this?

class MyClass # Mentioning MyClass in plain text won't be linked by default
  # :autolink: false
end

For not linking to specific words, like Ruby, Set....etc., they have a global effect in the project. And with that in mind, I think a centralized config like #1259 will be easier to maintain and understand.

Using directives for it could have a few downsides:

  • It's easier to create unexpected side-effects that are hard to debug.
    • For example, RDoc used to use :main: in its doc and it unexpectedly changed ruby/ruby's main page too, which was why Restore options after option parsing #426 workaround was needed later.
    • If any default gem added :autolink: to, say Kernel, for its own documentation, then ruby/ruby would suddenly stopped linking to Kernel too once the code was synced.
  • It's harder for developers to understand why a class/module isn't linked as they need to know that this directive exists. They'll eventually find it for sure after going through RDoc's docs, but a config in .rdoc_options should be easier to find in most cases.

So for ease of maintenance and understanding, I still prefer #1259 as the solution to #1254

@nobu
Copy link
Member Author

nobu commented Jan 1, 2025

  • If any default gem added :autolink: to, say Kernel, for its own documentation, then ruby/ruby would suddenly stopped linking to Kernel too once the code was synced.

The inverse can be true.
A gem, for instance set defines a class Set, and suddenly all word Set begins linking.

  • It's harder for developers to understand why a class/module isn't linked as they need to know that this directive exists. They'll eventually find it for sure after going through RDoc's docs, but a config in .rdoc_options should be easier to find in most cases.

There is unexpected linking risk, too.

In general, WiKi style autolinking is a sweet trap, for Ruby that capitalized words have the special meaning at least, I think.
We should encourage the use of explicit code linking, +Kernel+ etc, and should stop autolinking in plain text part in the future.

@nobu nobu force-pushed the autolink-directive branch from 956c7fb to ee5f96a Compare January 1, 2025 01:12
Comment on lines 434 to 439
autolink = @metadata["autolink"]
if autolink
RDoc::Options.boolean(autolink, "autolink")
else
true
end
Copy link
Member

Choose a reason for hiding this comment

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

Can we simplify this?

Suggested change
autolink = @metadata["autolink"]
if autolink
RDoc::Options.boolean(autolink, "autolink")
else
true
end
RDoc::Options.boolean(@metadata["autolink"] || true, "autolink")

Copy link
Member Author

Choose a reason for hiding this comment

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

fetch may be better?

Suggested change
autolink = @metadata["autolink"]
if autolink
RDoc::Options.boolean(autolink, "autolink")
else
true
end
RDoc::Options.boolean(@metadata.fetch("autolink", true), "autolink")

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@st0012
Copy link
Member

st0012 commented Jan 1, 2025

In general, WiKi style autolinking is a sweet trap, for Ruby that capitalized words have the special meaning at least, I think.
We should encourage the use of explicit code linking, +Kernel+ etc, and should stop autolinking in plain text part in the future.

I agree that this will be a better long-term direction 👍 But given this could be a breaking change, can you open an issue to expand this idea and we can discuss about the steps needed to get there?

@nobu
Copy link
Member Author

nobu commented Jan 1, 2025

What I don't like is we have to manage excluded words, like #1266, to add to each ".rdoc_options" file.

@nobu nobu force-pushed the autolink-directive branch 2 times, most recently from d4d5a4d to aeb7529 Compare January 1, 2025 13:27
@nobu nobu force-pushed the autolink-directive branch from aeb7529 to afb19dd Compare January 2, 2025 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow projects to disable auto-linking on certain words
3 participants