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

Jump to bottom

modify yardopts parsing to ignore hash and c-style comments #1070

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −2
Open

modify yardopts parsing to ignore hash and c-style comments #1070

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
7 changes: 5 additions & 2 deletions lib/yard/cli/yardopts_command.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,14 @@ def yardopts_options(opts)

private

# Parses the .yardopts file for default yard options
# Parses the .yardopts file for default yard options,
# ignoring comment lines beginning with // or #
# @return [Array<String>] an array of options parsed from .yardopts
def yardopts(file = options_file)
return [] unless use_yardopts_file
File.read_binary(file).shell_split
contents = File.read_binary(file)
opts = contents.gsub(%r{(#|//).*\n?}, "")
Copy link
Owner

@lsegal lsegal Feb 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We're going to need a requirement that comments start at the beginning of the line (incl. optional whitespace), otherwise a valid option like the following will not work:

--title "# a title!"

Or even (awkward but completely valid):

lib/*/*.rb
lib//*.rb

Unfortunately this would be a hard requirement.

I would recommend tests for these edge cases too.

Copy link
Author

Choose a reason for hiding this comment

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

I'm completely for the constraint that this only support comments starting at the beginning of the line.

I'm happy to add more tests -- wanted to get your temperature on this first.

opts.shell_split
rescue Errno::ENOENT
[]
end
Expand Down
13 changes: 13 additions & 0 deletions spec/cli/yardoc_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,10 +510,23 @@ def foo; end
optsdata = String.new("foo bar")
expect(optsdata).to receive(:shell_split)
expect(File).to receive(:read_binary).with("test").and_return(optsdata)
allow(optsdata).to receive(:gsub).and_return(optsdata)
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this mocked?

Copy link
Author

@eprothro eprothro Feb 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The optsdata string is frozen in this any many other tests. Since shell split is an extension on String I can't mock there, and I didn't want to couple the test any further (e.g. make it more integrated and lose what we actually care about here, that the string is parsed through shell_split)

@yardoc.options_file = "test"
@yardoc.run
end

it "ignores .yardopts tokens that are comments" do
optsdata = String.new("--one-file --locale es # --locale en\n// --title not_my_title")
Copy link
Owner

Choose a reason for hiding this comment

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

This test should not pass, see above. Having "#" as an argument to YARD is completely valid.

Copy link
Owner

Choose a reason for hiding this comment

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

Another bunch of completely valid examples:

--default-return #read

--query "object.path.include?('#')"

Copy link
Author

Choose a reason for hiding this comment

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

I added the "handle comments ending a line" functionality as an edge case at the last minute. Didn't think about these -- of course, you're right. Happy to not have this.

expect(File).to receive(:read_binary).with("test").and_return(optsdata)
@yardoc.options_file = "test"
@yardoc.run

expect(@yardoc.options.onefile).to be_truthy
expect(@yardoc.options.locale).to eq('es')
expect(@yardoc.options.title).not_to eq('not_my_title')
expect(@yardoc.options.serializer.options)
end

it "allows opts specified in command line to override yardopts file" do
expect(File).to receive(:read_binary).with(".yardopts").and_return("-o NOTMYPATH")
@yardoc.run("-o", "MYPATH", "FILE")
Expand Down