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

Allow customizing path prefix through options #1330

Merged
merged 1 commit into from
Mar 29, 2025

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Mar 24, 2025

In #1304, I removed the ability to set path prefix through patching Darkfish generator. But it turns out that it's used in sdoc.

See #1304 (comment)

But the original implementation was brittle and confusing. So instead of just restoring it, I think allowing the customization through options is a better approach.

Copy link

cloudflare-workers-and-pages bot commented Mar 24, 2025

Deploying rdoc with  Cloudflare Pages  Cloudflare Pages

Latest commit: 271fff0
Status: ✅  Deploy successful!
Preview URL: https://227da1f8.rdoc-6cd.pages.dev
Branch Preview URL: https://allow-customizing-path-prefi.rdoc-6cd.pages.dev

View logs

@st0012 st0012 force-pushed the allow-customizing-path-prefix-through-options branch from 1515099 to 76f604c Compare March 24, 2025 21:42
@st0012
Copy link
Member Author

st0012 commented Mar 24, 2025

@jonathanhefner I think with these changes sdoc can adopt the branch rather easily?

(Ignore Gemfile diffs)

diff --git a/Gemfile b/Gemfile
index 0802717..8b0e609 100644
--- a/Gemfile
+++ b/Gemfile
@@ -8,9 +8,11 @@ gem "hoe"
 gem "minitest"
 gem "puma"
 
-if ENV["rdoc"] == "master"
-  gem "rdoc", :github => "ruby/rdoc"
-end
+# if ENV["rdoc"] == "master"
+#   gem "rdoc", :github => "ruby/rdoc"
+# end
 
 gem "importmap-rails"
 gem "railties"
+
+gem "rdoc", :github => "ruby/rdoc", branch: "allow-customizing-path-prefix-through-options"
diff --git a/lib/sdoc/generator.rb b/lib/sdoc/generator.rb
index 023d066..884cb53 100644
--- a/lib/sdoc/generator.rb
+++ b/lib/sdoc/generator.rb
@@ -73,6 +73,9 @@ class RDoc::Generator::SDoc
     end
     @options.pipe = true
 
+    @options.class_module_path_prefix = "classes"
+    @options.file_path_prefix = "files"
+
     @original_dir = Pathname.pwd
     @template_dir = Pathname(options.template_dir)
     @output_dir = Pathname(@options.op_dir).expand_path(options.root)
diff --git a/lib/sdoc/rdoc_monkey_patches.rb b/lib/sdoc/rdoc_monkey_patches.rb
index fac1297..c8ab744 100644
--- a/lib/sdoc/rdoc_monkey_patches.rb
+++ b/lib/sdoc/rdoc_monkey_patches.rb
@@ -1,19 +1,5 @@
 require "rdoc"
 
-RDoc::TopLevel.prepend(Module.new do
-  def path
-    File.join("files", super)
-  end
-end)
-
-
-RDoc::ClassModule.prepend(Module.new do
-  def path
-    File.join("classes", super)
-  end
-end)
-
-
 RDoc::TopLevel.prepend(Module.new do
   attr_writer :path
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index ada0ca7..a7acfb1 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -28,7 +28,11 @@ def rdoc_top_level_for(ruby_code)
   # foolproof way to initialize it is by simply running it with a dummy file.
   $rdoc_for_specs ||= rdoc_dry_run("--files", __FILE__)
 
-  $rdoc_for_specs.store = RDoc::Store.new(RDoc::Options.new)
+  options = RDoc::Options.new
+  options.class_module_path_prefix = "classes"
+  options.file_path_prefix = "files"
+
+  $rdoc_for_specs.store = RDoc::Store.new(options)
 
   Dir.mktmpdir do |dir|
     path = "#{dir}/ruby_code.rb"

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

In #1304, I removed the ability to set
path prefix through patching Darkfish generator. But it turns out
that it's used in `sdoc`.

See #1304 (comment)

But the original implementation was brittle and confusing. So instead of
just restoring it, I think allowing the customization through options is
a better approach.
@st0012 st0012 force-pushed the allow-customizing-path-prefix-through-options branch from 76f604c to 271fff0 Compare March 25, 2025 10:56
@jonathanhefner
Copy link
Contributor

@st0012

I think with these changes sdoc can adopt the branch rather easily?

Yes, something like that could work.

Would these *_path_prefix options be released in RDoc 6.13.1 or in 6.14.0? If they are in 6.13.1, then I think the monkey patch can be dropped. However, if they are in 6.14.0, I think the monkey patch would need to be conditionally applied to support 6.13.x. And in that case, I'm more inclined to just use the monkey patch instead of the *_path_prefix options, to avoid additional code paths.

@st0012
Copy link
Member Author

st0012 commented Mar 25, 2025

I'm open to both 6.13.1 and 6.14 but prefer the latter as it's technically a new feature.

Can you give me more context on how sdoc manages:

  • Its dependency versions. Like how do you decide if it's ok to require rdoc >= 6.14
  • The Ruby versions it needs to support

@jonathanhefner
Copy link
Contributor

I'm open to both 6.13.1 and 6.14 but prefer the latter as it's technically a new feature.

Normally, I would agree that 6.14 is more appropriate. However, I think this feature arguably should have been a part of 6.13.0, so it seems reasonable to include it in 6.13.1. Also, 6.13.0 already deviated semantic versioning by including breaking changes; adding a feature in 6.13.1 isn't much more of a deviation.

Can you give me more context on how sdoc manages:

  • Its dependency versions. Like how do you decide if it's ok to require rdoc >= 6.14

  • The Ruby versions it needs to support

To be honest, I'm not sure if there is a policy for those things. I do not know what all is downstream of SDoc, so I am just trying to keep things in working order.

@st0012
Copy link
Member Author

st0012 commented Mar 26, 2025

I'm asking because we could focus on upstreaming/reducing sdoc's rdoc patches next, which I think will be beneficial to both sides. But if sdoc still needs to keep the patches around due to not being able to bump the required rdoc version, then it just won't be as effective.

@jonathanhefner
Copy link
Contributor

I'm asking because we could focus on upstreaming/reducing sdoc's rdoc patches next, which I think will be beneficial to both sides. But if sdoc still needs to keep the patches around due to not being able to bump the required rdoc version, then it just won't be as effective.

Ah, I see. In that case, I agree it would be beneficial to upstream some of the patches. All of the monkey patches apply idempotent changes, so even if SDoc can't drop them now, it won't be impacted by RDoc adopting them. And then SDoc could drop them at some point in the future.

List of patches:

@st0012
Copy link
Member Author

st0012 commented Mar 29, 2025

@jonathanhefner Thank you for the detailed list. I've created #1333 to remind ourselves to act on them later.
In the meantime, I'll merge this and cut 6.13.1.

@st0012 st0012 merged commit 3bc179d into master Mar 29, 2025
60 checks passed
@st0012 st0012 deleted the allow-customizing-path-prefix-through-options branch March 29, 2025 12:27
@st0012 st0012 mentioned this pull request Mar 29, 2025
st0012 added a commit that referenced this pull request Mar 29, 2025
Diff: v6.13.0...master

I consider #1330 a fix too because the new options are introduced as an
better alternative for a accidental breaking change.
@st0012
Copy link
Member Author

st0012 commented Mar 29, 2025

@jonathanhefner v6.13.1 is out with this PR.

@jonathanhefner
Copy link
Contributor

Thank you, @st0012! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants