You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/29 14:08:12 UTC

[GitHub] [arrow] eitsupi opened a new pull request, #13748: MINOR: [R] Add note about the deprecated `type()` function

eitsupi opened a new pull request, #13748:
URL: https://github.com/apache/arrow/pull/13748

   The `type()` function was deprecated in #12817 (ARROW-15168), but the documentation did not indicate that it had been deprecated.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r941273121


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   Somehow missed this before I merged the PR; I think it's pretty safe to say that this is a typo, so I'll open a follow-up PR.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] eitsupi commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r951040925


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @thisisnic My point is that you forgot to update the Rd file in #13822 or #13824 and merged #13824 without waiting for my reply, so this "pretty straightforward" problem is still not fixed even though a week has passed.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r941276849


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   https://github.com/apache/arrow/pull/13822



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r951055052


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @eitsupi I'm not sure exactly why you are continuing to argue with me about a trivial issue which has no practical consequences, but I'm done here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] eitsupi commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r950782408


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @thisisnic https://arrow.apache.org/docs/developers/reviewing.html says:
   
   > Arrow is a foundational project that will need to evolve over many years or even decades, while serving potentially millions of users. We believe that being meticulous when reviewing brings greater rewards to the project than being lenient and aiming for quick merges.
   
   > Reviewing is a communication between the contributor and the reviewer. Avoid letting questions or comments remain unanswered for too long (“too long” is of course very subjective, but two weeks can be a reasonable heuristic). If you cannot allocate time soon, do say it explicitly. If you don’t have the answer to a question, do say it explicitly. Saying “I don’t have time immediately but I will come back later, feel free to ping if I seem to have forgotten” or “Sorry, I am out of my depth here” is always better than saying nothing and leaving the other person wondering.
   
   You made the change without waiting for my reply (or ignoring my reply), did you not?
   
   I hope you will act as a matter of principle in the future. (Or I may have misunderstood something. In that case, I am very sorry.)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r950878764


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @eitsupi This PR is for a minor change, which I was keen to get merged quickly as it's pretty straightforward.  I already apologised above for assuming that the incorrect addition was a typo.  I am very familiar with those guidelines, having helped review and update the phrasing of them when they were being drafted.  
   
   I can only assume from your comment about me ignoring your reply (I did not  - things happen out of sync when you have multiple browser windows open), and the way that this conversation has continued over what appears to be a pretty trivial misunderstanding, that you believe I have a problem with you or your PRs.  This is not the case - honestly, it's great to have more people involved in the project, and it's really helpful to have more eyes on things like docs which are important but easily missed.  
   
   That said, it's clear that our interactions are not a positive experience for either of us, and so in future I'm going to avoid reviewing your PRs so that they can instead be reviewed by other members of the community.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic closed pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic closed pull request #13748: MINOR: [R] Add note about the deprecated `type()` function
URL: https://github.com/apache/arrow/pull/13748


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r943388493


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @eitsupi No worries about the crossed wires - FWIW it's not a priority to keep the Rd file looking the same.  Thanks again for this contribution :)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] eitsupi commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r941275309


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   This is intentional.
   The first line is used as the title (so it does not end in a comma) and the second line is the body.
   
   https://github.com/eitsupi/arrow/blob/67a7596f43098d5f899547b47332f6e710a5036c/r/man/infer_type.Rd#L6-L22



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r941499195


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   Ah, my mistake, sorry for assuming it was a typo there!  I'm going to remove the duplication in another PR - I see where you're coming from about having a title and description, but it's unnecessary to duplicate it and not consistent with other function docs in the package.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] eitsupi commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r942267029


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   ![image](https://user-images.githubusercontent.com/50911393/183874565-d3eb8bb4-a4f5-40ca-a3aa-b2ba7fafc2f5.png)
   https://arrow.apache.org/docs/8.0/r/reference/infer_type.html
   
   This was not introduced by me but was originally that way.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r942287036


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   That's just how pkgdown renders it if there is a title, but no description - see the Roxygen header for `read_schema()` versus how it appears on the website for another example of this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r951055052


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @eitsupi I'm not sure exactly why you are continuing to argue with me about a trivial issue, but I'm done here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] thisisnic commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
thisisnic commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r951053826


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @eitsupi I'm done here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] nealrichardson commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
nealrichardson commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r941270111


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   Is this line intentionally duplicated?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] eitsupi commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r941275309


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   This is intentional.
   The first line is used as the title (so it does not end in a comma) and the second line is the description.
   
   https://github.com/eitsupi/arrow/blob/67a7596f43098d5f899547b47332f6e710a5036c/r/man/infer_type.Rd#L6-L22



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] eitsupi commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r943383112


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @thisisnic This has nothing to do with pkgdown, it is the behavior of roxygen2.
   Please check the Rd file diff of this Pull Request.
   My intention was to keep the original Rd file as intact as possible. It is sad that this intention was not communicated and was considered an inappropriate update.
   
   BTW you seem to have forgotten to update the Rd file in #13824.
   This needs to be corrected.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [arrow] eitsupi commented on a diff in pull request #13748: MINOR: [R] Add note about the deprecated `type()` function

Posted by GitBox <gi...@apache.org>.
eitsupi commented on code in PR #13748:
URL: https://github.com/apache/arrow/pull/13748#discussion_r950782408


##########
r/R/type.R:
##########
@@ -58,6 +58,10 @@ FLOAT_TYPES <- c("float16", "float32", "float64", "halffloat", "float", "double"
 
 #' Infer the arrow Array type from an R object
 #'
+#' Infer the arrow Array type from an R object.

Review Comment:
   @thisisnic https://arrow.apache.org/docs/developers/reviewing.html says:
   
   > Arrow is a foundational project that will need to evolve over many years or even decades, while serving potentially millions of users. We believe that being meticulous when reviewing brings greater rewards to the project than being lenient and aiming for quick merges.
   
   > Reviewing is a communication between the contributor and the reviewer. Avoid letting questions or comments remain unanswered for too long (“too long” is of course very subjective, but two weeks can be a reasonable heuristic). If you cannot allocate time soon, do say it explicitly. If you don’t have the answer to a question, do say it explicitly. Saying “I don’t have time immediately but I will come back later, feel free to ping if I seem to have forgotten” or “Sorry, I am out of my depth here” is always better than saying nothing and leaving the other person wondering.
   
   You made the change without waiting for my reply (or ignoring my reply), did you not?
   
   I hope you will act as like this principle in the future. (Or I may have misunderstood something. In that case, I am very sorry.)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org