Skip to content

[SPARK-20889][SparkR] Grouped documentation for MISC column methods#18448

Closed
actuaryzhang wants to merge 4 commits intoapache:masterfrom
actuaryzhang:sparkRDocMisc
Closed

[SPARK-20889][SparkR] Grouped documentation for MISC column methods#18448
actuaryzhang wants to merge 4 commits intoapache:masterfrom
actuaryzhang:sparkRDocMisc

Conversation

@actuaryzhang
Copy link
Copy Markdown
Contributor

@actuaryzhang actuaryzhang commented Jun 28, 2017

What changes were proposed in this pull request?

Grouped documentation for column misc methods.

@actuaryzhang
Copy link
Copy Markdown
Contributor Author

@felixcheung @HyukjinKwon
Easiest group to update by far.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 28, 2017

Test build #78770 has finished for PR 18448 at commit 00d8bd8.

  • This patch fails due to an unknown error code, -10.
  • This patch merges cleanly.
  • This patch adds no public classes.

@actuaryzhang
Copy link
Copy Markdown
Contributor Author

jenkins, test this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 28, 2017

Test build #78814 has finished for PR 18448 at commit 00d8bd8.

  • This patch fails due to an unknown error code, -10.
  • This patch merges cleanly.
  • This patch adds no public classes.

@actuaryzhang
Copy link
Copy Markdown
Contributor Author

image

Copy link
Copy Markdown
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

LGTM - few minor comments

Comment thread R/pkg/R/functions.R
#' Miscellaneous functions defined for \code{Column}.
#'
#' @param x Column to compute on. In \code{sha2}, it is one of 224, 256, 384, or 512.
#' @param y Column to compute on.
Copy link
Copy Markdown
Member

@felixcheung felixcheung Jun 29, 2017

Choose a reason for hiding this comment

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

probably not only in this PR ... since y always go first, should we flip this order I think? list y first?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think roxygen automatically chooses the order of the arguments based on the order they appear in the file, and ignores the order we specify. So even if I move y before x here, in the generated doc, x will still appear before y. Indeed, as you can see from the screenshot, ... appears before y.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hmm

Comment thread R/pkg/R/functions.R Outdated
#'
#' @param x Column to compute on. In \code{sha2}, it is one of 224, 256, 384, or 512.
#' @param y Column to compute on.
#' @param ... additional columns.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: capital Columns to indicate type

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated now.

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 29, 2017

Test build #78854 has finished for PR 18448 at commit 203be11.

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

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 29, 2017

Test build #78865 has finished for PR 18448 at commit ff27f18.

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

@SparkQA
Copy link
Copy Markdown

SparkQA commented Jun 29, 2017

Test build #78916 has finished for PR 18448 at commit 35c5c4a.

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

@felixcheung
Copy link
Copy Markdown
Member

merged to master.

@asfgit asfgit closed this in fddb63f Jun 30, 2017
@actuaryzhang actuaryzhang deleted the sparkRDocMisc branch June 30, 2017 04:40
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.

3 participants