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

Remove call stack section #158

Closed
wants to merge 3 commits into from

Conversation

c-martinez
Copy link
Contributor

The Call Stack section of the python counterpart of this lesson has been removed from the lesson and moved to the discussion page.

As mentioned in this PR, it would be good to know whether instructors teaching this lesson tend to skip this section and thus it could be removed?

My guess is that, ideally, we would like the R and Python lessons to be as similar as possible (except when it comes to language-specific lessons). Is this right?

@jdblischak
Copy link
Contributor

Thanks for the PR, @c-martinez. I agree we should remove this section from the main lesson. While instructive for reading, it is difficult to teach in a workshop setting. In r-novice inflammation, we often put supplemental content in "supp" files. Could you please put the call stack section in a new section titled 02-supp-call-stack.Rmd? Then add a callout section in 02-func-R.Rmd that links to this supplemental content. Please let me know if you have any questions.

@c-martinez
Copy link
Contributor Author

Hi @jdblischak , I've added the 02-supp-call-stack.Rmd fine and a .callout section. I hope this is fine now.

@jdblischak
Copy link
Contributor

It's looking good, @c-martinez. Three minor things.

Each file needs a YAML header so that it is properly rendered online. Please add the following to the very top of the file 02-supp-call-stack.Rmd.

---
layout: page
title: Programming with R
subtitle: The call stack
minutes: 15
---

The callout section as it is has no context. The word "call stack" is not used before this point, so the title "Further details" is not correct since no details on the call stack have been given. Could you please title the callout box "The call stack" and then give a brief description? Maybe something like "For a deeper understanding of how functions work, you'll need to learn how they create their own environments and call other functions. Function calls are managed via the call stack. For more details on the call stack, have a look at the supplementary material."

The call stack is still included in the key points at the bottom of the lesson. Could you please move this key point to the supplementary file?

@c-martinez
Copy link
Contributor Author

Hi @jdblischak, I've incorporated the changes you suggested. I hope this is fine now.

> + We previously wrote functions called `fence` and `outside`.
> Draw a diagram showing how the call stack changes when we run the
> following:
> ```{r, results="hide"}
Copy link
Contributor

Choose a reason for hiding this comment

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

from the knitr documentation: hide hides results; this option only applies to normal R output (not warnings, messages or errors)

if we actually execute the below block of code in this document, we have an error:
## Error in eval(expr, envir, enclos): could not find function "outside"

maybe use eval=FALSE instead?

@chendaniely chendaniely added this to the Version 5.4 milestone Jun 6, 2016
chendaniely added a commit to chendaniely/r-novice-inflammation that referenced this pull request Jun 14, 2016
chendaniely added a commit to chendaniely/r-novice-inflammation that referenced this pull request Jun 14, 2016
@chendaniely chendaniely mentioned this pull request Jun 14, 2016
chendaniely added a commit that referenced this pull request Jun 14, 2016
rgaiacs added a commit to rgaiacs/swc-r-novice-inflammation that referenced this pull request Apr 17, 2018
zkamvar pushed a commit that referenced this pull request May 1, 2023
zkamvar pushed a commit that referenced this pull request May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants