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

Fix double return tag. #1226

Merged
merged 1 commit into from Mar 31, 2019
Merged

Conversation

thomthom
Copy link
Contributor

Description

In our C/C++ code base where we implement Ruby functions we some times have explicit @return [Boolean] tags for predicate methods.

YARD is adding an extra @return [Boolean] regardless if there already is one.

/**
 * This method is used to determine if a bounding box contains a specific
 * Point3d or BoundingBox object.
 *
 * @example
 *   boundingbox = Geom::BoundingBox.new
 *   boundingbox.add([100, 200, -400], [200, 400, 100])
 *   # This will return false.
 *   boundingbox.contains?([300, 100, 400])
 *   # This will return true.
 *   boundingbox.contains?([150, 300, -200])
 *
 * @overload contains?(point_or_bb)
 *
 *   @param [Geom::Point3d, Geom::BoundingBox] point_or_bb
 *
 * @return [Boolean]
 *
 * @version SketchUp 6.0
 */
static VALUE _wrap_contains(VALUE self, VALUE arg) {
  // ...
}

image

See output: http://ruby.sketchup.com/Geom/BoundingBox.html#contains%3F-instance_method

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).

@@ -213,6 +213,7 @@
foo = Registry.at('Foo#foo?')
expect(foo.docstring).to eq 'DOCSTRING'
expect(foo.tag(:return).types).to eq ['Boolean']
expect(foo.tags(:return).size).to eq 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the fix to handler_methods.rb this test still passed. And I don't quite understand why. I would have expected this to yield 2 and fail when YARD added an extra @return tag.

Copy link
Owner

Choose a reason for hiding this comment

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

There are no extra tags on the method defined in this specific test, which would explain why the result hasn't changed.

@@ -226,6 +227,7 @@
eof
foo = Registry.at('Foo#foo?')
expect(foo.tag(:return).types).to eq ['String']
expect(foo.tags(:return).size).to eq 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the test that catches the bug. I can dig deeper into why the other test didn't raise any errors before the fix.

@coveralls
Copy link

coveralls commented Feb 25, 2019

Coverage Status

Coverage increased (+0.003%) to 93.737% when pulling bc94da3 on thomthom:dev-fix-double-return-tag into 87ef6d8 on lsegal:master.

@@ -67,7 +67,9 @@ def handle_method(scope, var_name, name, func_name, _source_file = nil)
register_visibility(obj, visibility)
find_method_body(obj, func_name)
obj.explicit = true
obj.add_tag(Tags::Tag.new(:return, '', 'Boolean')) if name =~ /\?$/
if name =~ /\?$/ && obj.tags(:return).empty?
Copy link
Owner

Choose a reason for hiding this comment

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

This implementation should probably be normalized to handle overloads as per the Ruby handler:

https://github.com/lsegal/yard/blob/master/lib/yard/handlers/ruby/method_handler.rb#L41-L49

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any existing patterns to sharing logic between Ruby and C handlers?
Or should I just copy+paste?

@thomthom
Copy link
Contributor Author

I took another stab at this. Added a Common module under handlers for share logic.

I'll squash it if you are ok with these changes.

@@ -0,0 +1,18 @@
# frozen_string_literal: true
# Shared functionality between Ruby and C method handlers.
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this will add a docstring to the YARD::Handlers module, not MethodHandler.

Copy link
Owner

Choose a reason for hiding this comment

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

You will also want at least one newline after the frozen directive

Copy link
Owner

@lsegal lsegal left a comment

Choose a reason for hiding this comment

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

This looks good minus the docstring issue.

@thomthom
Copy link
Contributor Author

I corrected the docstring and rebased to latest YARD master branch.

@lsegal lsegal merged commit 64fe028 into lsegal:master Mar 31, 2019
@lsegal
Copy link
Owner

lsegal commented Mar 31, 2019

Great thanks for your work on this!

lsegal added a commit that referenced this pull request Apr 2, 2019
@thomthom thomthom deleted the dev-fix-double-return-tag branch April 9, 2019 18:48
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