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

Challenge 1 Solution - Issue #603 #713

Closed
wants to merge 4 commits into from
Closed

Challenge 1 Solution - Issue #603 #713

wants to merge 4 commits into from

Conversation

ammaraziz
Copy link
Contributor

This should solve #603 . The following changes were made:

  • Modified the first solution to be more consist with the other solutions by adding Step 1:.. Step 2: etc.
  • Modified "Now do the same for 2012" to "Do the same for 2012" so it's more clear what the learner needs to do.
  • Removed reference to any() to the bottom and labelled as alternative solutions
  • Added solution that uses %in%.

First (proper) pull request so please let me know if anything is wrong or needs correcting.

Ammar

Instructions

Thanks for contributing! ❤️

If this contribution is for instructor training, please email the link to this contribution to
checkout@carpentries.org so we can record your progress. You've completed your contribution
step for instructor checkout by submitting this contribution!

Keep in mind that lesson maintainers are volunteers and it may take them some time to
respond to your contribution. Although not all contributions can be incorporated into the lesson
materials, we appreciate your time and effort to improve the curriculum. If you have any questions
about the lesson maintenance process or would like to volunteer your time as a contribution
reviewer, please contact The Carpentries Team at team@carpentries.org.

You may delete these instructions from your comment.

- The Carpentries

This should solve #603 . The following changes were made:

- Modified the first solution to be more consist with the other solutions by adding `Step 1:.. Step 2:` etc. 
- Modified "Now do the same for 2012" to "Do the same for 2012" so it's more clear what the learner needs to do. 
- Removed reference to `any()` to the bottom and labelled as alternative solutions
- Added solution that uses `%in%`.
> > print("Record(s) for the year 2002 found.")
> > }
> > ```
> > Using the `any()` function, the logical condition can be expressed as:
> > ```{r ch10pt5-sol, eval=FALSE}
Copy link
Contributor

Choose a reason for hiding this comment

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

The chunk name should be updated so it is not a duplicate of the chunk name on line 133.

Copy link
Contributor Author

@ammaraziz ammaraziz Apr 29, 2021

Choose a reason for hiding this comment

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

Apologies, submitted correction to chunk name. Apologies for the multiple commits, I noticed my error too late.

Thank you.

@jcoliver
Copy link
Contributor

Thanks, @ammaraziz . Could you update the PR so there are no duplicate chunk names in the markdown file?

@jcoliver jcoliver added status:changes requested Waiting for Contributor to update PR type:clarification Suggest change for make lesson clearer type:enhancement Propose enhancement to the lesson and removed type:clarification Suggest change for make lesson clearer labels Apr 27, 2021
@jcoliver
Copy link
Contributor

Thanks for the update, @ammaraziz . At some point in the process, the formatting for the solution has gone awry. If you look at the Files changed tab for this PR or the file on your branch (https://github.com/ammaraziz/r-novice-gapminder/blob/patch-1/_episodes_rmd/07-control-flow.Rmd), note that the formatting is off starting around line 134. Most notably, the double greater than signs are missing > >, which are important for formatting (compare to the original version of the file at https://github.com/swcarpentry/r-novice-gapminder/blob/main/_episodes_rmd/07-control-flow.Rmd to see how it should be formatted). It might be easiest to start a new pull request, but I'll leave it up to you.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:changes requested Waiting for Contributor to update PR type:enhancement Propose enhancement to the lesson
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants