PoC for the event-based plugin system with YARD parsing plugin #1321

okuramasafumi · 2025-03-20T22:12:44Z

This PR introduces the event-based plugin system along with a YARD-parsing plugin as an example of concrete plugins.

It demonstrates how we can build practical plugins while keeping the core code small. In this case, YARD-related logic is encapsulated in a plugin and used only when users specify with --plugins option.
This YARD plugin successfully parses https://github.com/okuramasafumi/alba comments written with YARD, but doesn't print YARD-specific information such as types since current RDoc doesn't support type information.

This PR is completely proof of concept, and is not meant to be merged now.
I'd like to discuss if this is the right way to extend RDoc for developers, and even for core team, and improve maintainability.

kou · 2025-03-21T04:12:48Z

lib/rdoc/options.rb

+      opt.on("--plugins=PLUGINS", "-P", Array, "Use plugins") do |value|
+        @plugins.concat value
+      end


We can use multiple --plugin to add multiple plugins.

Suggested change

opt.on("--plugins=PLUGINS", "-P", Array, "Use plugins") do |value|

@plugins.concat value

end

opt.on("--plugin=PLUGIN", "-P", "Use plugin") do |plugin|

@plugins << plugin

end

I'm not sure which interface is better.

# This rdoc --plugins yard_plugin,another_plugin # Or that rdoc --plugin yard_plugin --plugin another_plugin

Correct me if I'm wrong, but I think the later way is kind of rare.

What does "rare" mean here?
The latter is used in many cases. For example, ruby's -I is the latter interface. curl's --data/--header/... are also the latter interface.

The latter style is easyer to use from a script:

args=() if [ "$USE_X_PLUGIN" = "yes" ]; then args+=(--plugin X_plugin) fi if [ "$USE_Y_PLUGIN" = "yes" ]; then args+=(--plugin Y_plugin) fi rdoc "${args[@]}" ...

@kou Thank you for correcting me, after some investigation, I noticed that we have both styles in Ruby and other tools.
Personally I don't have a strong opinion here. For the future version we can consider YAML configuration as a primary way to enable plugins (like rubocop).
I'm not against using multiple --plugin option here.

kou · 2025-03-21T04:17:25Z

lib/rdoc/rdoc.rb

@@ -449,6 +455,9 @@ def document options
    end
    @options.finish

+    ::RDoc::BasePlugin.activate_with(self)
+    @options.load_plugins


If we load plugins dynamically and use require, plugins will not be loaded to multiple RDoc instance.

Do we need to load plugins for each RDoc::RDoc#document?

Agree, initialize is a better place?

#initialize is better than #document but do we need to load plugins for each RDoc instance?
Can we load plugins only once in one process?

I cannot see that using multiple RDoc::RDoc instance is common. From CLI or Rake task, I think we have only one instance of RDoc::RDoc.
However, we might have multiple instances in the future, probably for concurrency.
So then, where is the ideal place to activate plugins? I through it's RDoc::Rdoc since it's in the execution path anyway.

gem install A B C creates a RDoc::RDoc for each package.

kou · 2025-03-21T04:19:54Z

lib/rdoc/event_registry.rb

We already have a plugin system in RDoc::Markup::PreProcess.

Could you explain what is the merit of "the event-based plugin system" you suggested (and problems in the existing system)? Should existing RDoc::Markup::PreProcess be deprecated by this?

I believe that RDoc::Markup::PreProcess requires adding directives to existing code, and that is a problem. One of the advantages of a new plugin system is that just enabling plugins works without modifying existing code.

Should existing RDoc::Markup::PreProcess be deprecated by this?

Using directives has its own advantages, so I'd like to keep it.

It seems that RDoc::Markup::PreProcess has the "post_process" hook too:

rdoc/lib/rdoc/markup/pre_process.rb

Lines 25 to 32 in 894b2f1

##

# Adds a post-process handler for directives. The handler will be called

# with the result RDoc::Comment (or text String) and the code object for the

# comment (if any).

def self.post_process &block

@post_processors << block

end

Should we deprecate it by suggested plugin system?

As far as I can see, this PreProcess works with directives, and this time it's not I want to work with. The goal of this PR is to introduce a way to make RDoc understand YARD without modifying existing code.
That said, we don't have to (or should not) deprecate post_process here since it's about directives and removing it will be backward-incompatible, and cannot be replaced by the plugin system in this PR.

Hmm. It seems that the "post_process" hook isn't depend on directives.

Could you try the following?

RDoc::Markup::PreProcess.post_process do |comment, code_object| if code_object.is_a?(RDoc::AnyMethod) puts "Parsing #{code_object.name}" end end

It seems that the hook is called for all methods.

kou · 2025-03-21T04:26:37Z

lib/rdoc/yard_plugin.rb

+          # meth.params = parsed_comment.param.map(&:to_s).join("\n")
+          meth.comment.text = parsed_comment.plain.join("\n")


Plugins should update the given objects destructively?
How to extend RDoc by a plugin when existing RDoc objects don't have a place to store additional data? In this case, how to use the parsed type information?

Yes I consider this and don't have a confident conclusion now.
I think there are two ways, one is using env object and another is modifying existing objects (meth in his case).
Using env would be safer, and storing type information would look like:

# In a plugin env[meth] = DataFromYardPlugin.new(type: type) # In a generator type = env[meth].type

Here, DataFromYardPlugin contains various information along with type.

If there are multiple plugins that want to attach additional information to meth, the approach may be fragile. (One plugin may overwrite env[meth] set by another plugin.)

Can we provide more safer mechanism?

kou · 2025-03-21T04:33:45Z

lib/rdoc/yard_plugin.rb

+          current_indentation_level = line[/^#\s*/]&.size || 0
+          if current_indentation_level >= @base_indentation_level + 2


Does this work when the first line is # ...?

Honestly this indentation-related implementation is not perfect and will fail for many edge cases. I'll definitely add test cases for this.

kou · 2025-03-21T04:34:54Z

lib/rdoc/yard_plugin.rb

+      }
+      def initialize(comment)
+        @comment = comment
+        @parsed_comment = ParsedComment.new([], nil, [], [])


It seems that we can use a local variable not an instance variable for this.

True, thank you for pointing out.

tompng · 2025-03-21T11:03:02Z

lib/rdoc/yard_plugin.rb

+                desc = matchdata[:desc1] || matchdata[:desc2]
+                parsed_type = TypeParser.parse(type)
+                @parsed_comment[:param] << ParamData.new(type: parsed_type, name: name, desc: desc)
+                @mode = :param


Storing previous_tag here instead of storing mode = :param, appending part will be simple

tag = ParamData.new(type: parsed_type, name: name, desc: desc) @parsed_comment[:param] << tag previous_tag = tag

# Append to the previous tag - data = @mode == :param || @mode == :raise ? @parsed_comment[@mode].last : @parsed_comment[@mode] - data.append_desc(line) + previous_tag.append_desc(line)

Awesome, I'll take this!

tompng · 2025-03-21T11:06:07Z

lib/rdoc/yard_plugin.rb

+        type_name: /#\w+|((::)?\w+)+/,
+        literal: /(?:
+           '(?:\\'|[^'])*' |
+           "(?:\\"|[^"])*" |


I think some regexps are wrong.
"\" (escaped, quote is not closed) matches to this string literal regexp.
Integer and Float literal(integer part) matches to type_name: /#\w+|((::)?\w+)+/

Oh I didn't notice this. I tested this code with some variations of type and it seems working. Tests are missing and we need to add them to ensure this regex working correctly since it's quite complex.

st0012 · 2025-03-23T19:24:52Z

lib/rdoc/base_plugin.rb

+    # Without calling this, plugins won't work
+
+    def self.activate_with(rdoc = ::RDoc::RDoc.current)
+      @@rdoc = rdoc


Using class variable for this means any later plugin calls activate_with with its own RDoc will apply that to all other plugins. I don't think this is what we want?

class Base @@name = "Base" def name @@name end def self.update_name(name) @@name = name end end class Foo < Base end class Bar < Base end Foo.update_name("Foo") f = Foo.new Bar.update_name("Bar") b = Bar.new puts f.name # => "Bar" puts b.name # => "Bar"

I thought it's safe to use a class variable here since we don't have to update rdoc object.
However, some information such as a name of the plugin, should be stored as a class instance variables.

If we don't expect plugins to call this method, then I don't think it should be in the base class as it is.

It doesn't look like the yard plugin needs this attribute yet? If that's the case, let's not introduce this until we have a real use case to help us define the API better.

If we want to pass RDoc's states down to plugins, let's avoid passing the RDoc object unless we have a solid use case for it. There's a lot of unnecessary coupling between RDoc's major components and I have been untangling for a while (example). Exposing RDoc directly will potentially make it harder for such work as all of its attributes & methods will be considered public APIs.

You are right, I didn't notice that it's not used at all now. I used it at first, but store has enough information for the plugin so I removed it.
Then we can remove this class variable. And I agree, we should avoid RDoc::RDoc object (thank you for untangling it!)

Yeah I'd expect we expose Options and Store objects first if we need to share states with plugins.

https://github.com/ruby/rdoc/pull/1321/files#diff-2458b7fcdf31ccd44fa0681ba7e381360b3e4623c9f5409358222e4826c3c60dR6

Sorry it turned out that we do use rdoc object here because we need some global data store for event handling.
I still agree that we should avoid RDoc::RDoc instance, but then where should we put these event registration logic? Maybe we can introduce something like PluginManager and pass it to plugins?

okuramasafumi force-pushed the parse-yard branch from f2bd262 to 5a2f695 Compare March 20, 2025 22:27

okuramasafumi added 2 commits March 21, 2025 07:30

feat(plugin): introduce simple event-based plugin system

91fe5e9

feat(plugin): introduce YardPlugin as an example plugin

9f0b3ad

okuramasafumi force-pushed the parse-yard branch from 5a2f695 to 9f0b3ad Compare March 20, 2025 22:30

kou reviewed Mar 21, 2025

View reviewed changes

tompng reviewed Mar 21, 2025

View reviewed changes

st0012 reviewed Mar 23, 2025

View reviewed changes

st0012 requested review from flavorjones, colby-swandale and vinistock March 23, 2025 20:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC for the event-based plugin system with YARD parsing plugin #1321

PoC for the event-based plugin system with YARD parsing plugin #1321

okuramasafumi commented Mar 20, 2025

kou Mar 21, 2025

okuramasafumi Mar 21, 2025

kou Mar 21, 2025

okuramasafumi Mar 23, 2025

kou Mar 21, 2025

okuramasafumi Mar 21, 2025

kou Mar 21, 2025

okuramasafumi Mar 23, 2025

kou Mar 24, 2025

kou Mar 21, 2025

okuramasafumi Mar 21, 2025

kou Mar 21, 2025

okuramasafumi Mar 23, 2025

kou Mar 24, 2025

kou Mar 21, 2025

okuramasafumi Mar 23, 2025

kou Mar 24, 2025

kou Mar 21, 2025 •

edited

Loading

okuramasafumi Mar 21, 2025

kou Mar 21, 2025

okuramasafumi Mar 21, 2025

tompng Mar 21, 2025 •

edited

Loading

okuramasafumi Mar 21, 2025

tompng Mar 21, 2025

okuramasafumi Mar 21, 2025

st0012 Mar 23, 2025

okuramasafumi Mar 23, 2025

st0012 Mar 23, 2025

okuramasafumi Mar 23, 2025

st0012 Mar 23, 2025

okuramasafumi Mar 23, 2025

	##
	# Adds a post-process handler for directives. The handler will be called
	# with the result RDoc::Comment (or text String) and the code object for the
	# comment (if any).

	def self.post_process &block
	@post_processors << block
	end

		# meth.params = parsed_comment.param.map(&:to_s).join("\n")
		meth.comment.text = parsed_comment.plain.join("\n")

		current_indentation_level = line[/^#\s*/]&.size \|\| 0
		if current_indentation_level >= @base_indentation_level + 2

PoC for the event-based plugin system with YARD parsing plugin #1321

Are you sure you want to change the base?

PoC for the event-based plugin system with YARD parsing plugin #1321

Conversation

okuramasafumi commented Mar 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tompng Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou Mar 21, 2025 •

edited

Loading

tompng Mar 21, 2025 •

edited

Loading