fix(options): RDoc::Options needs to call parse for its setup

[okuramasafumi]

This change workarounds the issue that RDoc::Options.new does not work itself without calling parse since some important setup logic lives in it.
I believe we can improve this a lot by moving default option setup in initialize, for example, but it takes time.
This usage of RDoc::Options.new is documented in README, and I believe this workaround solves it quickly and well enough.

[st0012]

If the given RDoc::Options is already "parsed", calling parse on it again with an empty array could override its existing values.

For example:

irb(main):015> option = RDoc::Options.new
=> 
#<RDoc::Options:0x0000000110ef6198
...
irb(main):016> ENV["RDOCOPT"] = "--all"
=> "--all"
irb(main):017> option.parse([]);
irb(main):018> option.visibility
=> :private
irb(main):019> option.visibility=:public
=> :public
irb(main):020> option.visibility
=> :public
irb(main):021> option.parse([]);
irb(main):022> option.visibility
=> :private
irb(main):023> 

It'll be a relatively rare to happen, but will be super hard to debug. So I'd like to avoid it if possible.

I think we may add some checks like:

class RDoc::Options
  def parsed?
    !!@option_parser
  end
end

@options = options.parse([]) unless options.parsed?

[okuramasafumi]

It'll be a relatively rare to happen

When it happens, is it intentional? If not, we can simply make parse no-op after calling it once.

def parse argv
  return if @option_parser
  # ...
end

[kou]

Why do we need to use a "workaround" approach here?

I think that we should not have the "parsed check". A user is responsible for calling parse how many times. We should not a fallback check for a failure case.

Can we fix this without a workaround approach something like the following?

diff --git a/lib/rdoc/options.rb b/lib/rdoc/options.rb
index a50ea806..92c6e6ef 100644
--- a/lib/rdoc/options.rb
+++ b/lib/rdoc/options.rb
@@ -378,7 +378,7 @@ class RDoc::Options
     @dry_run = false
     @embed_mixins = false
     @exclude = []
-    @files = nil
+    @files = []
     @force_output = false
     @force_update = true
     @generator = nil
@@ -620,7 +620,7 @@ class RDoc::Options
     # formatter
 
     unless @template then
-      @template     = @generator_name
+      @template     = @generator_name || default_generator_name
       @template_dir = template_dir_for @template
     end
 
@@ -1194,7 +1194,7 @@ Usage: #{opt.program_name} [options] [names...]
       opt.separator nil
     end
 
-    setup_generator 'darkfish' if
+    setup_generator default_generator_name if
       argv.grep(/\A(-f|--fmt|--format|-r|-R|--ri|--ri-site)\b/).empty?
 
     deprecated = []
@@ -1215,8 +1215,8 @@ Usage: #{opt.program_name} [options] [names...]
     end
 
     unless @generator then
-      @generator = RDoc::Generator::Darkfish
-      @generator_name = 'darkfish'
+      @generator = default_generator
+      @generator_name = default_generator_name
     end
 
     if @pipe and not argv.empty? then
@@ -1365,6 +1365,15 @@ Usage: #{opt.program_name} [options] [names...]
     end
   end
 
+  private
+  def default_generator
+    RDoc::Generator::Darkfish
+  end
+
+  def default_generator_name
+    "darkfish"
+  end
+
   ##
   # Loads options from .rdoc_options if the file exists, otherwise creates a
   # new RDoc::Options instance.

[tompng]

I don't think this is a bug to be fixed. see https://github.com/ruby/rdoc/pull/1325/files#r2009419651

It can be an improvement but there is a room for discussion in the API design.
The API design can be one of these:

• Works without configuration. For example, default development DB can be db/database.sqlite.
• Misconfiguration should fail. For example, default production database set to localhost:3306 is not so beneficial.

I think using RDoc::Options is a professional use API. The design is currently the latter type, and changing to the former is an API design decision issue. Is this change really useful? I feel fixing the README is enough.

[kou]

Can we close in favor of #1337 ?

[tompng]

README is fixed in #1337