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.

Sign up for GitHub

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/rdoc/rdoc.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ def remove_unparseable files

def document options
if RDoc::Options === options then
@options = options
@options = options.parse [] # Some logic only lives in `parse` method such as default generator
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

else
@options = RDoc::Options.load_options
@options.parse options
Expand Down
11 changes: 11 additions & 0 deletions test/rdoc/test_rdoc_rdoc.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,17 @@ def setup
@rdoc.instance_variable_set :@stats, @stats
end

def test_document_with_bare_options
rdoc = RDoc::RDoc.new
options = RDoc::Options.new
temp_dir do
rdoc.document options
end
store = rdoc.store
assert_equal nil, store.main
assert_equal nil, store.title
end

def test_document # functional test
options = RDoc::Options.new
options.files = [File.expand_path('../xref_data.rb', __FILE__)]
Expand Down
Loading