ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

ARROW-14441: [R] Add our philosophy to the dev vignette - #11705

Closed
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev
Closed

ARROW-14441: [R] Add our philosophy to the dev vignette#11705
thisisnic wants to merge 2 commits into
apache:masterfrom
thisisnic:ARROW-14713_split_dev

Conversation

@thisisnic

@thisisnicthisisnic commented Nov 15, 2021

Copy link
Copy Markdown
Member
  • separates some of the developer docs into separate files (content remains unchanged)
  • updates the original dev docs page to point to these other pages and discuss our philosophy when implementing bindings

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@thisisnic
thisisnic marked this pull request as ready for review November 17, 2021 16:04
Comment threadr/vignettes/developing.Rmd Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think this vignette needs more after here but I don't know exactly what. Maybe something on writing bindings between compute kernels and R functions? Or is that a bit too specific?

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Great point, agreed on the reference PR. I'm also gonna tag some of the wider R dev team to pitch in on this, as I can't help but feeling there's a bit more to discuss on what docs we distribute with the package vs. what docs we just have on the pkgdown site.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jonkeane What are your thoughts on this?

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.

Personally, I think of the pkgdown site as the canonical documentation, especially for vignettes. So I don't have a strong opinion / I'm not worried about not including it inside of / distributed along side the package.

As for examples: I think that would be great. We can link to PRs (though if the link goes to CRAN it's susceptible to a redirect/rot that will anger CRAN — we should and do check for that, but just a reminder). Though sometimes when writing examples I find it a little bit easier to make a dedicated example that has lots of extra comments/commentary/possibly even glosses over some of the reality involved with implementing them.

@wjones127wjones127 left a comment

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.

Thanks for organizing this! These developer docs are already quite nice and I'm glad to see them further enhanced.

Saw one broken link, and then a few other suggestions.

Comment threadr/vignettes/developers/setup.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated
Comment on lines 45 to 16

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.

I think these could be relative links:

Suggested change
*[setting up a development environment and building the components that make up the Arrow project and R package](https://arrow.apache.org/docs/r/articles/developers/setup.html)
*[common Arrow dev workflow tasks](https://arrow.apache.org/docs/r/articles/developers/workflow.html)
*[running R with the C++ debugger attached](https://arrow.apache.org/docs/r/articles/developers/debugging.html)
*[setting up a development environment and building the components that make up the Arrow project and R package](developers/setup.html)
*[common Arrow dev workflow tasks](developers/workflow.html)
*[running R with the C++ debugger attached](developers/debugging.html)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Unfortunately not - when the package is built, everything in the vignettes directory is distributed with the package as a HTML document, whereas everything in any subdirectories is only displayed on the pkgdown site. This means that the relative links wouldn't work for anyone viewing this vignette locally.

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.

Ohhh. Does this mean we are making parts of these developer docs not available offline?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, there's no real need to distribute them with the package given that most people read this content via the pkgdown site anyway.

Comment threadr/vignettes/developing.Rmd Outdated
Comment threadr/vignettes/developing.Rmd Outdated

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.

I think an example of a compute kernel binding would be good. We could use that as an example to further explain the above points.

I'd also suggest linking to recent PRs that may serve an examples. I find a reference PR particularly helpful in reminding me which files I might need to modify.

Comment threadr/vignettes/developers/setup.Rmd Outdated

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.

While we're in here, could we fold in the spirit of https://issues.apache.org/jira/browse/ARROW-14371?

Specifically:

  • A note that the brew-based method and the build-your-own methods are incompatible (mostly because of ARROW_HOME, but folks using brew shouldn't need to even think about that, so we should be careful how we phrase this)
  • A note about confirming that brew install apache-arrow --HEAD completes successfully and how to confirm it's being picked up in the install process
  • This might be part of the point above, but a description of what the difference/meaning of the following are: *** Using Homebrew ${PKG_BREW_NAME}, *** Arrow C++ libraries found via pkg-config, any mention of autobrew. Specifically, if one is trying to use homebrew for development, only the first one is ok, if someone sees something else that means that something isn't quite right.

If this is too much or you don't want to extend scope, that's totally fine!

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Will leave this for now as I don't fully understand all of these things, so am leaving it for another PR

Comment threadr/vignettes/developers/workflow.Rmd Outdated
@thisisnic

thisisnic commented Nov 19, 2021

Copy link
Copy Markdown
MemberAuthor

@jonkeane and @wjones127 - just rebased this so now the setup instructions contain the excellent changes made by @wjones127 including all that Windows content. Just thinking - I am in the process of writing up some content on writing bindings, but it's a bigger piece of work. Any objections to me doing that in a separate follow-up ticket, so we can potentially make other changes to the setup doc without having to rebase again?

@wjones127wjones127 left a comment

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.

Yeah I think doing the example as a follow-up makes sense.

@thisisnic

Copy link
Copy Markdown
MemberAuthor

I'll cover the bindings stuff in https://issues.apache.org/jira/browse/ARROW-14757

@jonkeane

Copy link
Copy Markdown
Member

@github-actions crossbow submit test-r-devdocs

@github-actions

Copy link
Copy Markdown

Revision: 1eec3a0

Submitted crossbow builds: ursacomputing/crossbow @ actions-1168

TaskStatus
test-r-devdocsGithub Actions

@jonkeane

Copy link
Copy Markdown
Member

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

@jonkeanejonkeane left a comment

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.

LGTM, pending the crossbow job. If that passes feel free to merge

@thisisnic

Copy link
Copy Markdown
MemberAuthor

This looks good. Just to confirm, the contents of r/vignettes/developers/workflow.Rmd and r/vignettes/developers/setup.Rmd were all just copy/pasted and don't have substantive edits do they? If they do, would you mind pointing them out so we can make sure to take a look at them?

Also I presume you've run pkgdown locally to see that this all looks ok. NBD if it isn't perfect on the first merge, we've also got lots of time to look at it on the dev docs site before the 7.0.0 release

Yeah, no substantive edits and will make sure I run pkgdown to check it all before merging.

@ursabot

ursabot commented Nov 23, 2021

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 7a9738a and contender = e417fbf. e417fbf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.18% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thisisnic@jonkeane@ursabot@wjones127