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/15 12:18:58 UTC

[GitHub] [arrow] dragosmg opened a new pull request, #13620: ARROW-17084: [R] Install the package before linting

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

   The package should be installed before running `lintr::ling_package()` or `lintr::expect_lint_free()` (our case).


-- 
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] dragosmg commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   AFAIU `pak::pak()` and `pak::pkg_install()` are equivalent (https://pak.r-lib.org/reference/pak.html#details-1) and so are `pak::local_install()` and `pak::pkg_install("local::.")` (https://pak.r-lib.org/reference/local_install.html#details-1) 
   => `pak::local_install()` and `pak::pak("local::.")` are equivalent.
   
   Are you thinking that `pak::local_install()` is a bit more readable? I would think people modifying this code are somewhat familiar with `pak`, and thus they encountered the `"local::."` incantation. For me, the `local_` pattern is less readable as there is a bit of overlap and different meaning than `withr::local_`. I read `local_` as we're doing something locally (which involves a temporary state of something), whereas in this context it actually means we are installing the package located in the root directory (more about location). From this point of view, I think `"local::."` is less ambiguous.  



-- 
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] dragosmg commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   AFAIU `pak::pak()` and `pak::pkg_install()` are equivalent (https://pak.r-lib.org/reference/pak.html#details-1) and so are `pak::local_install()` and `pak::pkg_install("local::.")` (https://pak.r-lib.org/reference/local_install.html#details-1) 
   => `pak::local_install()` and `pak::pak("local::.")` are equivalent.
   
   Are you thinking that `pak::local_install()` is a bit more readable? I would expect people modifying this code are somewhat familiar with `pak`, and thus they encountered the `"local::."` incantation. For me, the `local_` pattern is less readable as there is a bit of overlap and different meaning than `withr::local_`. I read `local_` as we're doing something locally (which involves a temporary state of something), whereas in this context it actually means we are installing the package located in the root directory (more about location). From this point of view, I think `"local::."` is less ambiguous.  
   
   But happy to change if you think otherwise. 



-- 
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] dragosmg commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   AFAIU `pak::pak()` and `pak::pkg_install()` are equivalent (https://pak.r-lib.org/reference/pak.html#details-1) and so are `pak::local_install()` and `pak::pkg_install("local::.")` (https://pak.r-lib.org/reference/local_install.html#details-1) 
   => `pak::local_install()` and `pak::pak("local::.")` are equivalent.
   
   Are you thinking that `pak::local_install()` is a bit more readable? I would think people modifying this code are somewhat familiar with `pak`, and thus they encountered the `"local::."` incantation. For me, the `local_` pattern is less readable as there is a bit of overlap and different meaning than `withr::local_`. I read `local_` as we're doing something locally (which involves a temporary state of something), whereas in this context it actually means we are installing the package located in the root directory (more about location). From this point of view, I think `"local::."` is less ambiguous.  
   
   But happy to change if you think otherwise. 



-- 
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] jonkeane merged pull request #13620: ARROW-17084: [R] Install the package before linting

Posted by GitBox <gi...@apache.org>.
jonkeane merged PR #13620:
URL: https://github.com/apache/arrow/pull/13620


-- 
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] dragosmg commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   AFAIU `pak::pak()` and `pak::pkg_install()` are equivalent (https://pak.r-lib.org/reference/pak.html#details-1) and so are `pak::local_install()` and `pak::pkg_install("local::.")` (https://pak.r-lib.org/reference/local_install.html#details-1) => `pak::local_install()` and `pak::pak("local::.")` are equivalent.
   
   Are you thinking that `pak::local_install()` is a bit more readable? I would think people modifying this code are somewhat familiar with `pak`, and thus they encountered the `"local::."` incantation. For me, the `local_` pattern is less readable as there is a bit of overlap and different meaning than `withr::local_`. I read `local_` as we're doing something locally (which involves a temporary state of something), whereas in this context it actually means we are installing the package located in the root directory (more about location). From this point of view, I think `"local::."` is less ambiguous.  



-- 
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] dragosmg commented on pull request #13620: ARROW-17084: [R] Install the package before linting

Posted by GitBox <gi...@apache.org>.
dragosmg commented on PR #13620:
URL: https://github.com/apache/arrow/pull/13620#issuecomment-1195508166

   @jonkeane & @nealrichardson would you mind having a look?


-- 
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] jonkeane commented on pull request #13620: ARROW-17084: [R] Install the package before linting

Posted by GitBox <gi...@apache.org>.
jonkeane commented on PR #13620:
URL: https://github.com/apache/arrow/pull/13620#issuecomment-1202467777

   Interesting, thanks for that additional context. We should merge this fix regardless (since it's the recommended way of running lintr!), but I suspect that something else might have happened with the seemingly off files here. 
   
   I was curios, so went looking and turns out, the offending whitespace line was introduced in https://github.com/apache/arrow/pull/13610 which did get properly flagged: https://github.com/apache/arrow/runs/7349390401?check_suite_focus=true so at least that did work at the time.
   
   Anyway, like I said, I'll merge this since it's the right way to be running lintr according to the developers


-- 
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] ursabot commented on pull request #13620: ARROW-17084: [R] Install the package before linting

Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #13620:
URL: https://github.com/apache/arrow/pull/13620#issuecomment-1202766704

   Benchmark runs are scheduled for baseline = 48e27804296233a9ab90b6096e291046ff2db6f3 and contender = 51eb3c8adb5742f8d0d05c2e371dfbc651499614. 51eb3c8adb5742f8d0d05c2e371dfbc651499614 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
   Conbench compare runs links:
   [Failed :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/306937107d414201a9abbd82fd102972...f9fd4109faf4442cb0c800ca071e2b3f/)
   [Failed :arrow_down:0.37% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/5fafe3df696543218a6d307e44186403...465ae275df6e4e84b530b3d371c1009f/)
   [Failed :arrow_down:0.27% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/4535693b866d4f35836cb32e888475b3...49f35f9cc6b44f99b9b3c855a9483bf3/)
   [Finished :arrow_down:0.14% :arrow_up:0.04%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/e1dac593f9234ee888df7916af44e020...aac29f5195cc474098efa4d913dac889/)
   Buildkite builds:
   [Failed] [`51eb3c8a` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1239)
   [Failed] [`51eb3c8a` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1251)
   [Finished] [`51eb3c8a` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1233)
   [Finished] [`51eb3c8a` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1253)
   [Failed] [`48e27804` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/1238)
   [Finished] [`48e27804` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/1250)
   [Failed] [`48e27804` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/1232)
   [Finished] [`48e27804` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/1252)
   Supported benchmarks:
   ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
   test-mac-arm: Supported benchmark langs: C++, Python, R
   ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
   ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
   


-- 
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] jonkeane commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   Would `pak::local_install()` work here? https://pak.r-lib.org/reference/local_install.html Or at least a comment about why not | what it's doing?



-- 
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] dragosmg commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   I switched to `pak::local_install()` as your search experience points to that documentation being a bit more accessible. I also added a comment.



-- 
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] dragosmg commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   AFAIU `pak::pak()` and `pak::pkg_install()` are equivalent (https://pak.r-lib.org/reference/pak.html#details-1) and so are `pak::local_install()` and `pak::pkg_install("local::.")` (https://pak.r-lib.org/reference/local_install.html#details-1) => `pak::local_install()` and `pak::pak("local::.")` are equivalent.
   
   Are you thinking that `pak::local_install()` is a bit more readable? I would think people modifying this code are somewhat familiar with `pak`, and thus they encountered the `"local::."` incantation. For me, the `local_` pattern is less readable as there is a bit of overlap and different meaning than `withr::local_`. I read `local_` as we're doing something locally, whereas in this context it actually means we are installing the package located in the root directory. From this point of view, I think `"local::."` is less ambiguous.  



-- 
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] dragosmg commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   AFAIU `pak::pak()` and `pak::pkg_install()` are equivalent (https://pak.r-lib.org/reference/pak.html#details-1) and so are `pak::local_install()` and `pak::pkg_install("local::.")` (https://pak.r-lib.org/reference/local_install.html#details-1) 
   => `pak::local_install()` and `pak::pak("local::.")` are equivalent.
   
   Are you thinking that `pak::local_install()` is a bit more readable? I would expect people modifying this code are somewhat familiar with `pak`, and maybe they encountered the `"local::."` incantation. For me, the `local_` pattern is less readable as there is a bit of overlap and different meaning than `withr::local_`. I read `local_` as we're doing something locally (which involves a temporary state of something), whereas in this context it actually means we are installing the package located in the root directory (more about location). From this point of view, I think `"local::."` is less ambiguous.  
   
   But happy to change if you think otherwise. 



-- 
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] jonkeane commented on a diff in pull request #13620: ARROW-17084: [R] Install the package before linting

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


##########
.github/workflows/r.yml:
##########
@@ -327,6 +327,13 @@ jobs:
         shell: Rscript {0}
         working-directory: r
         run: |
+          Sys.setenv(
+            RWINLIB_LOCAL = file.path(Sys.getenv("GITHUB_WORKSPACE"), "r", "windows", "libarrow.zip"),
+            MAKEFLAGS = paste0("-j", parallel::detectCores()),
+            ARROW_R_DEV = TRUE,
+            "_R_CHECK_FORCE_SUGGESTS_" = FALSE
+          )
+          pak::pak("local::.")

Review Comment:
   I'm not familiar enough with {pak} and {pak}'s idioms to know which are the better or more natural ones for {pak}. https://pak.r-lib.org/reference/pak_package_sources.html#local-package-trees-1 lists `local_install()` first and it's easier to get to that from the reference section of the docs, which is what I first tried to do when I was trying to figure out what `pak::pak("local::.")` was doing while reviewing this (the docs for `pak::pak()` doesn't list what `"local::."` means, which was the first place I went). 
   
   I don't think that we can be certain people modifying this code will be familiar with {pak} (as far as I know this is the only place in the whole repo we use it). So we should flag what's up with a comment if we're using something that's not easy to get to the documentation about. We might even also add in why {pak} is helpful here over other approaches 
   



-- 
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] github-actions[bot] commented on pull request #13620: ARROW-17084: [R] Install the package before linting

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13620:
URL: https://github.com/apache/arrow/pull/13620#issuecomment-1185516262

   https://issues.apache.org/jira/browse/ARROW-17084


-- 
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] dragosmg commented on pull request #13620: ARROW-17084: [R] Install the package before linting

Posted by GitBox <gi...@apache.org>.
dragosmg commented on PR #13620:
URL: https://github.com/apache/arrow/pull/13620#issuecomment-1202301926

   @jonkeane Thanks for your review. I'll try to answer the questions below:
   * we do not currently have a CI job to test this. It's also a bit trickier to reproduce 
   * I noticed the failure (false-positive) during work on PR #13160 when the CI was picking up a lint in a file that did not exist in my branch - the CI run can still be seen [here](https://github.com/dragosmg/arrow/runs/7358965910?check_suite_focus=true), although the history of the branch was re-written due to a rebase. At the moment of the CI run, `test-dplyr-glimpse.R` did not exist in my branch, but it was in master.
   * another odd aspect (a false negative) is that the lint in `test-dplyr-glimpse.R` was not picked up as part of the PR in which it got added.  


-- 
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] dragosmg commented on pull request #13620: ARROW-17084: [R] Install the package before linting

Posted by GitBox <gi...@apache.org>.
dragosmg commented on PR #13620:
URL: https://github.com/apache/arrow/pull/13620#issuecomment-1202638699

    I see what you mean. I guess it depends on the point of view. If you look at it as the state of the package pre-merge (my branch) then the lint is a false-positive (which is AFAICT should be the case). If you look at it as the state of the package post-merge (the master) then the picked-up lint was _not_ a false-positive. 
   
   As you say, regardless of the POV, the authors' recommended workflow is to install the package before linting. 
   Installing the package, AFAIU, will lint the branch version of the package and not the master one. 
   
   If my understanding is correct, it points to a way we could test this. Introduce a lint in a new file in master. With the old workflow any subsequent PR should pick-up this lint (even if the affected file is not touched), while with the new CI workflow, the lint in master should not be picked-up (if the file isn't present in the branch).


-- 
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