ARROW-13403: [R] Update developing.Rmd vignette #10930

thisisnic · 2021-08-13T12:20:49Z

No description provided.

github-actions · 2021-08-13T12:21:08Z

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

thisisnic · 2021-08-16T11:19:50Z

CI failure is due to wider CI issues around a new Debian release, rather than a problem with the docs.

thisisnic · 2021-08-16T14:21:45Z

The main changes in this PR were:

removing some redundant content to bring the vignette length down a bit
re-ordering the content, and adding extra headings/subheadings to make it easier to find the relevant sections and signpost what content is where
move some of the detail about the internals of the configure script and other installation scripts to install.Rmd
renaming headings to make it easier to browse the vignette
add in a warning about Windows development#
change "we" to "you" where appropriate

…me content

…into install vignette

jonkeane

Thanks for all of this polishing. I think there are really great improvements here. I have a few comments + suggestions, the biggest one is if we do want to remove the tabbing from the developing vignette.

jonkeane · 2021-08-23T20:13:30Z

r/vignettes/developing.Rmd

+output: 
+  html_document:
+      toc: true
+      toc_depth: 2


I'm curious if there's a general movement away from rmarkdown::html_vignette? I've always used rmarkdown::html_vignette because I thought it was streamlined for inclusion in the package (and pkgdown overrides this stuff anyway for the web docs).

Oops, I chucked this in so it made it easier to preview the list of contents when I was rendering this as RMarkdown

jonkeane · 2021-08-23T20:13:52Z

r/vignettes/developing.Rmd

 vignette: >
  %\VignetteIndexEntry{Arrow R Developer Guide}
  %\VignetteEngine{knitr::rmarkdown}
  %\VignetteEncoding{UTF-8}
+


Super nit picky: is this new line needed?

I'll remove

r/vignettes/developing.Rmd

jonkeane · 2021-08-23T20:27:14Z

r/vignettes/developing.Rmd

-2. Building the Arrow library — this actually compiles the Arrow library
-3. Install the Arrow library — this organizes and moves the compiled Arrow library files into the location specified in the configuration
-4. Building the R package — this builds the C++ code in the R package, and installs the R package for you
+There are five major steps to the process — the first four are relevant to all Arrow developers, and the last one is specific to developers making changes to the R package.


This is a very minor point, so we might want to ignore it totally / gloss over it here, but technically "the first four are relevant to all Arrow developers" isn't quite true. There are (small) differences in building the cpp code between the languages that use it (for example the flags recommended for python development are slightly different — we could in principle recommend that folks use a union of the flags and then they would be able to do either, though compilation would be longer, so I'm not sure we want to do that). Additionally, other languages, like Rust, don't rely on the cpp code at all AFAIK, so for those arrow developers this isn't super relevant.

All of that said, it might be best to leave it as is, or say something like "are similar to steps that Arrow developers working on other languages use" and leave it be. The details above in this comment are definitely not relevant for someone building Arrow for R development for the first time

Thanks for highlighting this, I'll come up with a different way of phrasing this to be more accurate!

I've just deleted it as it doesn't really add anything.

jonkeane · 2021-08-23T20:31:07Z

r/vignettes/developing.Rmd


-### Install dependencies {.tabset}
+### Step 1 - Install dependencies


Did you intend to get rid of {.tabset} here? It's a bit funky how it works (and is very much non-standard/hacked on to pkgdown), but it gives us a nice tabbed interface for interacting with the setup steps (e.g. https://arrow.apache.org/docs/r/articles/developing.html#install-dependencies).

Yeah, my intention was to make it really easy to navigate through the page, using the table of contents that appears at the side, and the heading levels, to signpost what bits applies to what.

I could add it back in though - will see how including it affects the contents list.

If the goal is to keep the tab titles in the table of contents, it's possible we could do that by changing some of the javascript that we manipulate to make this whole thing work. Something funny that we do there is that we load the javascript for sticky tabs, and then alter some of the functions to do the things we need them to do in-place.

I had another think and actually, the tabset stuff is totally fine. When I built the docs and previewed them, I realised it doesn't actually matter to have those bits in the tables of contents. Good to know there are options there though

r/vignettes/developing.Rmd

jonkeane · 2021-08-23T20:42:12Z

r/vignettes/developing.Rmd

-``` shell
-R CMD build .
-R CMD check arrow_*.tar.gz --as-cran
+If rebuilding the Arrow library doesn't work and you are [installing from a user-level directory](#configure-for-installing-to-a-user-directory) and you already have a previous installation of libarrow in a system directory or you get you may get errors like the following when you install the R package:


Suggested change

If rebuilding the Arrow library doesn't work and you are [installing from a user-level directory](#configure-for-installing-to-a-user-directory) and you already have a previous installation of libarrow in a system directory or you get you may get errors like the following when you install the R package:

If rebuilding the Arrow library doesn't work and you are [installing from a user-level directory](#configure-for-installing-to-a-user-directory) and you already have a previous installation of libarrow in a system directory or you get errors like the following when you install the R package:

I'm not totally sure this is what you intended, but I think it is?

No idea, I think I just moved this section from elsewhere rather than rewriting it and had just skim-read it so didn't catch this. I'll rewrite the whole sentence to break it down a little.

Have now rephrased this section a little.

jonkeane · 2021-08-23T20:43:38Z

r/vignettes/developing.Rmd

+
+## Other installation issues
+
+There are a number of scripts that are triggered when the arrow R package is installed. For package users who are not interacting with the underlying code, these should all just work without configuration and pull in the most complete pieces (e.g. official binaries that we host). However, knowing about these scripts can help package developers troubleshoot if things go wrong in them or things go wrong in an install.  See [the installation vignette](./install.html) for more information.


Could/should we link to the How dependencies are resolved section in the install vignette here?

Sounds good

jonkeane · 2021-08-23T20:48:06Z

r/vignettes/install.Rmd

@@ -123,6 +123,39 @@ you'll need to reinstall the package in order to enable S3 support.

 # How dependencies are resolved

+There are a number of scripts that are triggered when `R CMD INSTALL .` is run. 
+For Arrow users, these should all just work without configuration and pull in 
+the most complete pieces (e.g. official binaries that we host).


I wonder if it would be good to flag that most of this information is really only intended to help developers and not day-to-day installers of Arrow? I don't mind (and actually kind of like!) that it's in the install vignette as opposed to the developing vignette, but most of the information is still probably targets at (and most relevant to) developers and not standard installers of Arrow.

There's also a reference at the end that says "(If you're looking at this script, and you've gotten this far, it might look
incredibly familiar: it's basically the contents of this guide in script form —
with a few important changes)" which we probably should link over to the developing vignette there.

I don't think we want to take out some of the content just because it's not relevant to installers (since it's useful info and we don't want to have two different largely overlapping sections in both vignettes)

Good point, will have a look at doing a better job of signposting who this is for and change up that later sentence!

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

thisisnic · 2021-08-25T09:42:44Z

When changing the R package C++, I've again run into the issue that something needs editing to make sure that the lint script uses Python 3 (I think, can't remember the specific cause). I'm going to add something to this PR which covers that (or open a ticket to do something to the script) as I won't be the only person to run into this. (Now have submitted a PR for this: #11002)

jonkeane

This looks good. I built it locally with pkgdown and re-reviewed it there. I caught a few typos (none of them introduced in this PR!) that I suggested corrections for.

jonkeane · 2021-09-01T20:58:22Z

r/vignettes/developing.Rmd

+* "Apache Arrow" or "Arrow" - the Apache Arrow project, including implementations in different languages
+* "libarrow" or "the Arrow C++ library" - Arrow's C++ library (N.B. this term might not be used in documentation in other parts of the Arrow project)
+* "arrow" or "the Arrow R package" - the R package
+* `arrow` - a directory into which the Arrow GitHub repo has been checked out


This is more polish than anything else, but I wonder if this style section would be better as a footnote? It's really good info, but it feels a bit of a sidestep right at the beginning.

Good point - whilst writing this down was useful while I was working on the vignette, now that terminology is consistent, it's a little redundant.

I'm not 100% on board with the info as a footnote as I think it's important to have it up front still, so what I think will satisfy all requirements here is just explain what libarrow is, in parentheses, after the first mention of the term, and then leave everything else to be implicit.

The main point of doing this was the libarrow vs. arrow package distinction, so I think we've ticked those boxes now - though happy to adjust again if you think further changes are needed.

r/vignettes/developing.Rmd

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

…c/arrow into ARROW-13403_developing_vignette

r/vignettes/developing.Rmd

Closes apache#10930 from thisisnic/ARROW-13403_developing_vignette Lead-authored-by: Nic Crane <thisisnic@gmail.com> Co-authored-by: Nic <thisisnic@gmail.com> Co-authored-by: Jonathan Keane <jkeane@gmail.com> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

thisisnic marked this pull request as draft August 13, 2021 12:21

github-actions bot added the Component: R label Aug 13, 2021

thisisnic marked this pull request as ready for review August 16, 2021 11:09

thisisnic added 11 commits August 23, 2021 21:00

Split content out some more

dc3d4ff

Add styling/linting section back in

1cbd7b1

Add back in other missing bit

3b14d84

Small tweaks

68e912f

Add extra headings to make it easier to navigate the docs, reorder so…

606c527

…me content

Fix internal links

01d28f3

Update pkgdown command to show preview

d7ff3dd

Remove section as it's not Arrow-specific knowledge

2e204c5

Rename section headers and move some of the info around installation …

e6df296

…into install vignette

Add back in the full validation bit

0b8261e

Rephrase "specific ref"

fd45fd7

thisisnic force-pushed the ARROW-13403_developing_vignette branch from 73ce7f2 to fd45fd7 Compare August 23, 2021 20:00

jonkeane reviewed Aug 23, 2021

View reviewed changes

thisisnic and others added 3 commits August 25, 2021 08:22

Update r/vignettes/developing.Rmd

c72fc8f

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

Update r/vignettes/developing.Rmd

62913d5

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

Update r/vignettes/developing.Rmd

e10c29d

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

thisisnic added 7 commits August 25, 2021 17:25

Remove newline and revert to using html_vignette

7d6f025

Add tabsets back in and a bit of rephrasing

4800da9

Remove system install instructions

55b9f46

Plural to singular

53d7e83

Rephrase longer sentence

edf659d

Link to install vignette

8fe5faa

Link to cpp11

9315f6f

github-actions bot added Component: C++ Component: Java Component: Python Component: Parquet labels Aug 26, 2021

arrow terminology

95aacf7

thisisnic force-pushed the ARROW-13403_developing_vignette branch from 3b34e88 to 95aacf7 Compare August 26, 2021 08:03

thisisnic added 2 commits August 26, 2021 09:14

Move the install script subsection and add a sentence about who it's for

2461b27

I have no idea where these changes came from

3cc6df7

thisisnic force-pushed the ARROW-13403_developing_vignette branch from daf3278 to 3cc6df7 Compare August 26, 2021 08:16

thisisnic added 2 commits August 26, 2021 08:18

Merge branch 'master' into ARROW-13403_developing_vignette

0a7b943

Shell to bash for nicer highlighting, split out inline code

6a34060

thisisnic requested a review from jonkeane August 26, 2021 09:48

Fix typo and whitespace

0e90fe6

jonkeane removed Component: Java Component: Parquet Component: C++ Component: Python labels Sep 1, 2021

jonkeane approved these changes Sep 1, 2021

View reviewed changes

thisisnic and others added 5 commits September 2, 2021 11:42

Update r/vignettes/developing.Rmd

aa3c9b2

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

Update r/vignettes/developing.Rmd

dc434f5

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

Move definition to footnote

1f44aca

Refactor a few things for grammar and clarity

fd55285

Merge branch 'ARROW-13403_developing_vignette' of github.com:thisisni…

5abf9b8

…c/arrow into ARROW-13403_developing_vignette

jonkeane reviewed Sep 6, 2021

View reviewed changes

r/vignettes/developing.Rmd Show resolved Hide resolved

Update r/vignettes/developing.Rmd

f3e3550

jonkeane closed this in 5876e3f Sep 6, 2021

asfimport mentioned this pull request Sep 6, 2021

[R] Update developing.Rmd vignette #29073

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-13403: [R] Update developing.Rmd vignette #10930

ARROW-13403: [R] Update developing.Rmd vignette #10930

thisisnic commented Aug 13, 2021

github-actions bot commented Aug 13, 2021

thisisnic commented Aug 16, 2021

thisisnic commented Aug 16, 2021

jonkeane left a comment

jonkeane Aug 23, 2021

thisisnic Aug 25, 2021

jonkeane Aug 23, 2021

thisisnic Aug 25, 2021

jonkeane Aug 23, 2021

thisisnic Aug 25, 2021

thisisnic Aug 25, 2021

jonkeane Aug 23, 2021

thisisnic Aug 25, 2021

jonkeane Aug 25, 2021

thisisnic Aug 25, 2021

jonkeane Aug 23, 2021

thisisnic Aug 25, 2021

thisisnic Aug 25, 2021

jonkeane Aug 23, 2021

thisisnic Aug 25, 2021

thisisnic Aug 25, 2021

jonkeane Aug 23, 2021

thisisnic Aug 25, 2021

thisisnic Aug 26, 2021

thisisnic commented Aug 25, 2021 •

edited

Loading

jonkeane left a comment

jonkeane Sep 1, 2021

thisisnic Sep 2, 2021


		### Install dependencies {.tabset}
		### Step 1 - Install dependencies

	If rebuilding the Arrow library doesn't work and you are [installing from a user-level directory](#configure-for-installing-to-a-user-directory) and you already have a previous installation of libarrow in a system directory or you get you may get errors like the following when you install the R package:
	If rebuilding the Arrow library doesn't work and you are [installing from a user-level directory](#configure-for-installing-to-a-user-directory) and you already have a previous installation of libarrow in a system directory or you get errors like the following when you install the R package:


		## Other installation issues

		There are a number of scripts that are triggered when the arrow R package is installed. For package users who are not interacting with the underlying code, these should all just work without configuration and pull in the most complete pieces (e.g. official binaries that we host). However, knowing about these scripts can help package developers troubleshoot if things go wrong in them or things go wrong in an install. See [the installation vignette](./install.html) for more information.

ARROW-13403: [R] Update developing.Rmd vignette #10930

ARROW-13403: [R] Update developing.Rmd vignette #10930

Conversation

thisisnic commented Aug 13, 2021

github-actions bot commented Aug 13, 2021

thisisnic commented Aug 16, 2021

thisisnic commented Aug 16, 2021

jonkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnic commented Aug 25, 2021 • edited Loading

jonkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisisnic commented Aug 25, 2021 •

edited

Loading