fix: NULL in mutate() may cause error and incorrect columns order#356
fix: NULL in mutate() may cause error and incorrect columns order#356Yousa-Mirage wants to merge 11 commits into
NULL in mutate() may cause error and incorrect columns order#356Conversation
etiennebacher
left a comment
etiennebacher
left a comment
There was a problem hiding this comment.
I still need to review the code in itself because it becomes really complicated to understand, but I already have a few comments to address below.
| expect_equal( | ||
| test_pl |> | ||
| mutate( | ||
| Species = Sepal.Width + 1, | ||
| foo = mean(Sepal.Length), | ||
| .by = Species, | ||
| ), | ||
| test_df |> | ||
| mutate( | ||
| Species = Sepal.Width + 1, | ||
| foo = mean(Sepal.Length), | ||
| .by = Species, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
What does this test add compared to the existing tests?
There was a problem hiding this comment.
This is just a regression prevention test that tests the behavior when the grouping variable is modified. You can delete it if you feel it is not necessary. The test in that comment below was really unnecessary and I've deleted it.
| expect_equal( | ||
| test_pl |> | ||
| group_by(Species) |> | ||
| mutate(Species = Sepal.Width + 1, foo = mean(Sepal.Length)) |> | ||
| ungroup(), | ||
| test_df |> | ||
| group_by(Species) |> | ||
| mutate(Species = Sepal.Width + 1, foo = mean(Sepal.Length)) |> | ||
| ungroup() | ||
| ) |
|
I agree with you, expecially for the group part. I spent time to understand the code generated by Codex. It uses a temp col to record the grouping columns. Maybe there are better methods to aovid the modify from affecting grouping. |
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
|
Well, the test failure just now was becase, |
# Conflicts: # R/mutate.R
2 participants
fix #355