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 link to return type #1

Closed
wants to merge 5 commits into from

Conversation

jimfoltz
Copy link
Contributor

@jimfoltz jimfoltz commented Jan 7, 2017

@thomthom
Copy link
Member

Thank Jim - the tool was able to merge back the changes correctly for this one. So far so good.

You want me to merge this in as-is?

@jimfoltz
Copy link
Contributor Author

You want me to merge this in as-is?

I'm not sure. Would you want to regenerate the stubs first for a comparison?

@thomthom
Copy link
Member

The re-generated stubs looks the same as the changes you submitted.

@jimfoltz
Copy link
Contributor Author

Yes.

@jimfoltz
Copy link
Contributor Author

jimfoltz commented Jan 15, 2017

Or did you mean to just keep a running branch for all pull requests? Either way - just do it whatever way is better for you as it doesn't really have an effect on my workflow.

# all of the entities in the active model, component, or group (if you are
# within a group or component edit session.)
#
# @example
# model = Sketchup.active_model
# entities = model.active_entities
#
# @return [Sketchup::Entities] entities - the {Sketchup::Entities} of the current editing context, or Model{Model#entities} if none.
# @return [Entities] The {Entities} of the current editing context,
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to use fully qualified identifiers? Sketchup::Entities instead of Entities? Or is just Entities working for the stubs? (In which case I need to verify it works when generating from our C++ source)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably choose the fully-qualified identifier as a convention if only for consistency.

#
# @return status - true if successful, false if unsuccessful
# @return [Boolean] +true+ if successful, +false+ if unsuccessful
Copy link
Member

Choose a reason for hiding this comment

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

For most boolean type of return I think @return [Boolean] is enough - the "true is successful is redundant". We only need to add as much info as needed.

# The transform! method is used to apply a transformation to a component
# instance.
# The +transform!+ method is used to apply a {Geom::Transformation} to a
# component instance.
Copy link
Member

Choose a reason for hiding this comment

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

Why the indent?

@@ -477,8 +477,8 @@ def split(instance)
def subtract(instance)
end

# The transform! method is used to apply a transformation to a component
# instance.
# The +transform!+ method is used to apply a {Geom::Transformation} to a
Copy link
Member

Choose a reason for hiding this comment

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

I've started denoting all references to classes and methods with the reference tag. In this case I would write:

The {#transform!} method ...

Copy link
Member

Choose a reason for hiding this comment

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

And on that topic I'm never been too keen on the "The method_name is doing x" way of introducing a method, I feel is echoing too much. However, it's used so consistently across the 1000+ methods already documented that there might not be any point trying to change that now?

@thomthom
Copy link
Member

I was thinking that contributors would make a pull request, which we'll then pull on our side and run the merge tool on. Then we'd close the PR.

@thomthom thomthom closed this in 49df372 Jan 16, 2017
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.

2 participants