modify yardopts parsing to ignore hash and c-style comments #1070
Conversation
There was a problem hiding this comment.
It looks like the behavior cannot be accepted as-is. Allowing comments in the middle of a line would create way too much parsing ambiguity. See the inline comments for examples of cases that tests don't cover.
For this to be accepted, the PR has to be modified to only ignore comments that begin a line, and a bunch more tests would be needed to exercise the "failing" cases.
| # @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?}, "") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @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") |
There was a problem hiding this comment.
This test should not pass, see above. Having "#" as an argument to YARD is completely valid.
There was a problem hiding this comment.
Another bunch of completely valid examples:
--default-return #read
--query "object.path.include?('#')"
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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)
lsegal
force-pushed
the
master
branch
2 times, most recently
from
44c07a8 to
112a890
Compare
Description
Adds support for hash and c-style comments in .yardopts files.
We want this to be able to reduces developer confusion for those new to YARD. Devs can tend to see a .yardopts file, get confused, then just shrug and walk away.
Being able to comment the options can pull them in more easily, and allow for documenting options that may not be use currently but are of interest.
Example
Completed Tasks
bundle exec rakelocally (if code is attached to PR).