Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add diagram description #445

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

@jspmccain
Copy link

@jspmccain jspmccain commented Aug 22, 2019

Wanted to address this issue described in: #372

I added some text below the figure describing what it is, and I also added some text to the 'Commenting' section of 09-supp-intro-rstudio.Rmd talking about the usefulness of commenting for describing functions.

@katrinleinweber
Copy link
Contributor

@katrinleinweber katrinleinweber commented Aug 26, 2019

Thanks for these suggestions @jspmccain & welcome to the Carpentries :-)

I'll review your PR this week.

Loading

@katrinleinweber katrinleinweber self-assigned this Aug 26, 2019
@katrinleinweber katrinleinweber self-requested a review Aug 26, 2019
Copy link
Contributor

@katrinleinweber katrinleinweber left a comment

Please definitely change the diagram-related addition. My 2nd comment about the... Commenting is secondary here & could lead to a separate PR.

Loading

@@ -147,6 +147,8 @@ Overall, your package directory should look something like this:

<img src="../fig/R-package-structure.svg" alt="R Package Structure" width="500" />

In the diagram above, your package is a folder (the same as it would exist on your computer!). This folder contains several files and sub-folders. For example, the folder 'R' contains all of the R scripts you just wrote above.

Copy link
Contributor

@katrinleinweber katrinleinweber Aug 27, 2019

Choose a reason for hiding this comment

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

Please try to integrate the info that your suggestion contains uniquely into the lines above the diagram, so that we don't need to add anything here. Since we don't have proper image captions, would like to avoid spreading info around the image.

Loading

@@ -62,7 +62,7 @@ window and press <kbd>Esc</kbd>; this should help you out of trouble.
### Commenting

Use `#` signs to comment. Comment liberally in your R scripts. Anything to the
right of a `#` is ignored by R.
right of a `#` is ignored by R. Commenting is also a great way to remind yourself of what certain functions you write do, and it helps others understand the purpose of your function without having to read the code.
Copy link
Contributor

@katrinleinweber katrinleinweber Aug 27, 2019

Choose a reason for hiding this comment

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

We have 4 sentences now, which seem to jump back and forth between technical details & purpose of commenting. Let's try to merge them, purpose first.

How about the following?

Comments remind yourself of what certain functions you write do, and it helps others understand their purpose without having to read the code. Start your comments with a # signs, which R takes as the sign to ignore anything to the right of it when running the code.

Loading

Copy link
Contributor

@katrinleinweber katrinleinweber Aug 27, 2019

Choose a reason for hiding this comment

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

If you agree & while we're at it, we could also add something like:

Also remember that expressive naming of functions, arguments and variables, as well as [#'-prefixed roxygen documentation]({{ page.root }}//08-making-packages-R/#making-your-first-r-package), document your code. Consider free-form, #-comments as a last resort.

Loading

Base automatically changed from master to main Mar 17, 2021
@HaoZeke
Copy link
Member

@HaoZeke HaoZeke commented Apr 19, 2021

Hi @jspmccain, would you be willing or able to revisit this to update the PR?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants