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

first use of function with side effects needs explaining? #385

Closed
steltenpower opened this issue Feb 6, 2022 · 3 comments · Fixed by #405
Closed

first use of function with side effects needs explaining? #385

steltenpower opened this issue Feb 6, 2022 · 3 comments · Fixed by #405
Labels
good first issue Good issue for first-time contributors

Comments

@steltenpower
Copy link
Contributor

On
levels(respondent_floor_type)[2] <- "brick"
a student said:"if you're not putting the result of the levels function into a variable, what's going on?"

@atheobold
Copy link
Contributor

@steltenpower -- I would agree that this is a bit hard to understand for beginners. Personally, I would like to see this changed to some of the functions from the forcats package, such as fct_relevel(). Would you be interested in putting in a pull request improving this section of the workshop content?

@atheobold atheobold added the good first issue Good issue for first-time contributors label Feb 9, 2022
@cgross95
Copy link
Contributor

If there is still any interest in making these changes throughout the episode, I would be happy to contribute a pull request!

If so, the same section uses, e.g., in line 395
respondent_floor_type <- factor(respondent_floor_type, levels = c("earth", "cement"))
to reorder the levels. If the forcats functions are preferable, would it be worth it to change this to
respondent_floor_type <- fct_relevel(respondent_floor_type, c("earth", "cement"))?

Since these commands use pretty much the same syntax, I would also understand if this change unnecessarily introduces the fct_relevel function and it would be better to leave it out.

@juanfung
Copy link
Contributor

@cgross95 that would be great! Since we have not introduced forcats yet, I would prefer the former but if you are feeling ambitious perhaps we could have a callout with a quick intro to forcats illustrating the alternative, and maybe some other uses. Either way, I'm happy with the explicit use of factor as in line 395 for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good issue for first-time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants