don't link to deprecated function#22370
Conversation
Seems misleading to (without qualification) link to a deprecated function
|
Can one of the admins verify this patch? |
felixcheung
left a comment
felixcheung
left a comment
There was a problem hiding this comment.
thanks for the your input. I'm not sure if this is a big problem - I think it's helpful to link from the deprecated method to the replacement, and back?
| #' @param ... additional named parameters as options for the data source. | ||
| #' @return A SparkDataFrame. | ||
| #' @rdname createTable | ||
| #' @seealso \link{createExternalTable} |
There was a problem hiding this comment.
One thing we should check for such things is, if it happens globally in the documentation or not BTW.
There was a problem hiding this comment.
I don't see it here (nothing pointing to sparkR.init/sparkRHive.init/sparkRSQL.init):
https://spark.apache.org/docs/latest/api/R/sparkR.session.html
or here (nothing pointing to dropTempTable):
https://spark.apache.org/docs/latest/api/R/dropTempView.html
But I do see it here (points to registerTempTable):
https://spark.apache.org/docs/latest/api/R/createOrReplaceTempView.html
There was a problem hiding this comment.
Shall I amend the PR to fix the registerTempTable page as well?
There was a problem hiding this comment.
If we should fix, yea, better fix them all. @felixcheung has a better insight in R side. You batter wait for his opinion.
There was a problem hiding this comment.
registerTempTable is because of the @family tag, so it's a bit different.
|
@felixcheung I disagree... what's the point of deprecation if it's going to keep being considered as a co-equal function in the eyes of documentation? If the function is being deprecated, it should be "soft-blacklisted" -- documentation still available (but clearly marked) for users of "legacy" code where its usage still survives, but I don't see any argument for even letting new users find out a deprecated function exists. |
|
I don’t feel strongly either way.
I do think this is very minor since there are still many other ways to the doc page for createExternalTable (eg the index page) or via ? search within R etc. I am not sure how much difference this would make and we have already a) code spewing out warning when called b) clearly documented as Deprecated on the doc page title.
Should you find other deprecation that is not documentation we should be gladly having your help
to document it.
|
felixcheung
left a comment
felixcheung
left a comment
There was a problem hiding this comment.
hey thanks for the contribution, I'm good if you could remove the @family in registerTempTable and check the doc generated by doc build?
| #' @param ... additional named parameters as options for the data source. | ||
| #' @return A SparkDataFrame. | ||
| #' @rdname createTable | ||
| #' @seealso \link{createExternalTable} |
There was a problem hiding this comment.
registerTempTable is because of the @family tag, so it's a bit different.
|
@felixcheung thanks for the feedback, yes, I axed Sorry I botched an attempt to try and rename the branch of this PR (apparently not possible to overwrite this PR with a new branch name); PR re-filed as #22393 |
Continuation of #22370. Summary of discussion there: There is some inconsistency in the R manual w.r.t. supercedent functions linking back to deprecated functions. - `createOrReplaceTempView` and `createTable` both link back to functions which are deprecated (`registerTempTable` and `createExternalTable`, respectively) - `sparkR.session` and `dropTempView` do _not_ link back to deprecated functions This PR takes the view that it is preferable _not_ to link back to deprecated functions, and removes these references from `?createOrReplaceTempView` and `?createTable`. As `registerTempTable` was included in the `SparkDataFrame functions` `family` of functions, other documentation pages which included a link to `?registerTempTable` will similarly be altered. Author: Michael Chirico <michael.chirico@grabtaxi.com> Author: Michael Chirico <michaelchirico4@gmail.com> Closes #22393 from MichaelChirico/axe_deprecated_doc_refs. (cherry picked from commit a1dd782) Signed-off-by: Felix Cheung <felixcheung@apache.org>
4 participants
Seems misleading to (without qualification) link to a deprecated function
What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
How was this patch tested?
(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)
Please review http://spark.apache.org/contributing.html before opening a pull request.