Skip to content

Conversation

@y-shimizu
Copy link
Contributor

I implemented toString for AssociationRules.Rule, format like [x, y] => {z}: 1.0

Copy link
Member

Choose a reason for hiding this comment

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

Trivial, but you should use @return and there's no need to state the return type again. Remove the blank line below too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen
Thank you for review.
I fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think Returns is better than @return. If we use @return, the message doesn't show up by default in the API doc, e.g.,

screen shot 2015-09-24 at 8 38 56 am

User needs to open it to see the doc:

screen shot 2015-09-24 at 8 39 05 am

If we use Returns, it shows up by default:

screen shot 2015-09-24 at 8 39 35 am

@srowen

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I don't feel strongly about it. Wouldn't this argument go for all return docs though? They're hidden by default for brevity. Maybe important ones should be put inline in the main doc but toString seems pretty minor.

Copy link
Contributor

Choose a reason for hiding this comment

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

For each method, there should be a summary sentence at the beginning, which shows up in the API doc by default. Other details are hidden by default. I think this is Scala specific. You can find the official guide at http://docs.scala-lang.org/style/scaladoc.html:

If the documentation of a method is a one line description of
what that method returns, do not repeat it with an @return annotation.

@y-shimizu
Copy link
Contributor Author

@mengxr I fixed format.

So, which should I use, Returns or @return ?

@mengxr
Copy link
Contributor

mengxr commented Sep 25, 2015

Returns. For toString, it is also okay to remove the JavaDoc entirely.

@y-shimizu
Copy link
Contributor Author

@mengxr I fixed.

@SparkQA
Copy link

SparkQA commented Sep 25, 2015

Test build #1815 has finished for PR 8904 at commit 1f9ce43.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Sep 25, 2015

We still need a return type on the method, and the line is too long and needs to be wrapped

@y-shimizu
Copy link
Contributor Author

@srowen I fixed.

@SparkQA
Copy link

SparkQA commented Sep 26, 2015

Test build #1816 has finished for PR 8904 at commit ae12fba.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 26, 2015

Test build #1817 has finished for PR 8904 at commit ae12fba.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Sep 27, 2015

Test build #1819 has finished for PR 8904 at commit ae12fba.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Sep 27, 2015

Merged to master

@asfgit asfgit closed this in 299b439 Sep 27, 2015
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.

4 participants