Skip to content
This repository has been archived by the owner on Jan 3, 2018. It is now read-only.

replaced paste() with cat(). #466

Closed
wants to merge 2 commits into from

Conversation

likit
Copy link

@likit likit commented Apr 28, 2014

added anonymous function.
added an example of how to return multiple values
replaced range() with span() according to Python tutorial.
edited/added some text.

added anonymous function.
added an example of how to return multiple values
replaced range() with span() similar to Python example.
edited/added some text.
@jdblischak
Copy link
Contributor

Thanks for this, @likit. Are you on the r-discuss mailing list? If not, please sign up here.

However, this is a little tricky to merge at the moment because these materials are being actively developed. We recently adopted a plan for translating the novice Python lessons to R (see this blog post). @gavinsimpson is currently making a round of changes (#452), which will then be followed by two others.

So I think the two main options would be:

  • Send a PR to @gavinsimpson's fork with these proposed changes
  • Wait until the three rounds of revisions are done before adding additional material

Thoughts on when to merge these changes and/or whether we should discuss anonymous functions in the novice lessons? @gavinsimpson or any other SWC R instructors?

@gavinsimpson
Copy link
Contributor

Thanks @likit.

Changing to cat() solves just one of my objections to the use of paste() etc to format output; you just don't use R this way, unnecessary typing etc. You can see more at #459

The consensus there was to drop all the formatting of strings/concatenation and just use comment for a note of what the calculation was and simply include the statement to be evaluated. I hadn't done this yet as this was only recently discussed here and I wanted time for people to comment. @jdblischak has just closed this (#459) so I intend to implement the change in a different way than you've done here.

IIRC I've also rewritten the section where range() is mentioned and this now more closely follows the Python lesson. But agreed; this section was very confusing until I went back to the Python version - then it made sense.

I'll take a look at the other changes you made - trade off between adding R-related concepts (e.g. your section on anonymous functions) vs length of lesson. For example, I've just added a longish section on writing for() loops well in R and how looping relates to the apply() family of functions. However, is this lesson now too long? Will see during the review.

(FYI I'm just giving it one last read through before I push my changes to 03-loops-R.Rmd so this won't show up in the Pull Request or my fork of the bc repo just yet.)

@jdblischak
Copy link
Contributor

Thanks for singing up for the r-discuss mailing list, @likit. Since you are now familiar with this lesson material, would you be able to give @gavinsimpson's PR a thorough review?

@gavinsimpson, I'll leave it to you to close this PR and to coordinate with @likit on the incorporation of any of these changes into your repo.

@gavinsimpson
Copy link
Contributor

@jdblischak I don't have permissions to close PRs on SWC.

I will go back through @likit's PR here and merge in where needed.

@likit
Copy link
Author

likit commented May 7, 2014

@gavinsimpson @jdblischak Thanks for comments. Sorry, I have been very busy these days because I'm about to graduate in August. I'll try to follow as much as possible but I won't be able to contribute much until after July.

@jdblischak
Copy link
Contributor

No worries, @likit. We appreciate your suggestions, and we'll do our best to incorporate them. Best of luck as you prepare to graduate!

@jdblischak jdblischak closed this May 13, 2014
@jdblischak jdblischak added the R label May 22, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants