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.

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

Add support for quoted symbols #1168

Merged
merged 3 commits into from May 28, 2018

Conversation

cboos
Copy link
Contributor

@cboos cboos commented May 19, 2018

Description

Constants containing quoted symbols can now be documented correctly.

Prior to the fix, the value :"sym+bol" would be shown as sym+bol
and worse, the empty symbol (:'') would have triggered an error in
MethodHelper#format_constant, as the corresponding constant value
was the empty String.

For example, this code:

class X
  Y = :''
end

would trigger the following error in YARD 0.9.12:

[error]: NoMethodError: undefined method `[]' for nil:NilClass
[error]: Stack trace:
        .../lib/yard/templates/helpers/method_helper.rb:69:in `format_constant'
        .../templates/default/module/html/constant_summary.erb:10:in `block in _erb_cache_18'
        .../templates/default/module/html/constant_summary.erb:6:in `each'
        .../templates/default/module/html/constant_summary.erb:6:in `_erb_cache_18'
        .../lib/yard/templates/template.rb:287:in `erb'
        .../lib/yard/templates/template.rb:365:in `render_section'

The ConstantHandler didn't treat the :dyna_symbol node specially, and
simply calling the source method on that statement would return the characters
making up the symbol, not the actual source code for that symbol (i.e. the prefix :
and the wrapping quotes).

This is fixed in commit 5b1dc8a.

Besides, when investigating the issue, I first followed a wrong track by fixing
an error in the tokenisation code of the parser. This proved to be not
necessary to fix my original error, but I think the change will not harm
so I've left it in the PR as a separate commit (db00597).

Some more details about this other fix: in RubyParser,
a sequence of tokens gets built, and a [:symbeg, :const] pair
gets replaced by a :symbol token.
However, there are actually three possibilities (maybe more?)
for constructing a Symbol:

  1. :symbeg, :const (e.g. :BAR)
  2. :symbeg, :tstring_content, :tstring_end (e.g. :"B+z")
  3. :symbeg, :tstring_end (e.g. :'')

So I added support for case 2. and 3. as well.

Completed Tasks

  • I have read the Contributing Guide.
  • The pull request is complete (implemented / written).
  • Git commits have been cleaned up (squash WIP / revert commits).
  • I wrote tests and ran bundle exec rake locally (if code is attached to PR).

cboos added 2 commits May 19, 2018 20:05
Without the fix, the new test "properly tokenizes symbols" would fail with:

```
  1) YARD::Parser::Ruby::RubyParser#parse properly tokenizes symbols
     Failure/Error: expect(symbols).to eq %w( :'' :BAR :"B+z" )

       expected: [":''", ":BAR", ":\"B+z\""]
            got: [":'", ":BAR", ":B+z"]

       (compared using ==)
```

which shows that symbols were indeed not reconstructed properly.
Constants containing quoted symbols can now be documented
correctly.

Prior to the fix, the value `:"sym+bol"` would be shown as `sym+bol`
and worse, the empty symbol (`:''`) would have triggered an error in
MethodHelper#format_constant.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 93.506% when pulling 5b1dc8a on cboos:format_constant-empty-symbol into d6491b4 on lsegal:master.

@coveralls
Copy link

coveralls commented May 19, 2018

Coverage Status

Coverage increased (+0.009%) to 93.506% when pulling 4fa86f3 on cboos:format_constant-empty-symbol into d6491b4 on lsegal:master.

@cboos
Copy link
Contributor Author

cboos commented May 19, 2018

Sorry, I did run 'rake rubocop' and 'rake spec', but forgot to run 'rake yard': now the latter fails. I'll investigate and provide a follow-up.

The sequence `:symbeg`, `:ident` (corresponding to symbols like `:ident`)
also needs to be taken into account.
@cboos
Copy link
Contributor Author

cboos commented May 20, 2018

Btw, as both the contributing guide and the PR template advise to do a bundle exec rake, what about including :yard as an extra step to the default build? Running YARD on itself is a nice last validation step on top of the tests!

@lsegal lsegal merged commit 12b9c41 into lsegal:master May 28, 2018
@lsegal
Copy link
Owner

lsegal commented May 28, 2018

Thank you for the PR.

I agree that running yard as part of the rake might be worthwhile, but I think we would need some kind of way to validate the results. Without validating, I'm not sure how this would work.

One thing we could do is integration test the yard doc command as a separate RSpec test and just smoke test for a valid exit code (0). That would be a reasonable test and would fit nicely into our testing infrastructure. I would accept a PR for this.

lsegal added a commit that referenced this pull request May 28, 2018
@cboos
Copy link
Contributor Author

cboos commented Jun 3, 2018

PR #1171 added a smoke test to run yard on yard's source code during CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants