nathaneastwood / poorman

A poor man's dependency free grammar of data manipulation
https://nathaneastwood.github.io/poorman/
Other
339 stars 15 forks source link

Fix: arrange() should reset rownames when they are numeric #102

Closed etiennebacher closed 2 years ago

etiennebacher commented 2 years ago

Closes #86.

Before:

suppressPackageStartupMessages({
  library(poorman)
})

# Should reset row names
df <- data.frame(x = head(letters))
arrange(df, -x)
#>   x
#> 6 f
#> 5 e
#> 4 d
#> 3 c
#> 2 b
#> 1 a

# Shouldn't reset row names
head(mtcars) %>% 
  arrange(mpg)
#>                    mpg cyl disp  hp drat    wt  qsec vs am gear carb
#> Valiant           18.1   6  225 105 2.76 3.460 20.22  1  0    3    1
#> Hornet Sportabout 18.7   8  360 175 3.15 3.440 17.02  0  0    3    2
#> Mazda RX4         21.0   6  160 110 3.90 2.620 16.46  0  1    4    4
#> Mazda RX4 Wag     21.0   6  160 110 3.90 2.875 17.02  0  1    4    4
#> Hornet 4 Drive    21.4   6  258 110 3.08 3.215 19.44  1  0    3    1
#> Datsun 710        22.8   4  108  93 3.85 2.320 18.61  1  1    4    1

Created on 2022-08-02 by the reprex package (v2.0.1)

After:

suppressPackageStartupMessages({
  library(poorman)
})

# Should reset row names
df <- data.frame(x = head(letters))
arrange(df, -x)
#>   x
#> 1 f
#> 2 e
#> 3 d
#> 4 c
#> 5 b
#> 6 a

# Shouldn't reset row names
head(mtcars) %>% 
  arrange(mpg)
#>                    mpg cyl disp  hp drat    wt  qsec vs am gear carb
#> Valiant           18.1   6  225 105 2.76 3.460 20.22  1  0    3    1
#> Hornet Sportabout 18.7   8  360 175 3.15 3.440 17.02  0  0    3    2
#> Mazda RX4         21.0   6  160 110 3.90 2.620 16.46  0  1    4    4
#> Mazda RX4 Wag     21.0   6  160 110 3.90 2.875 17.02  0  1    4    4
#> Hornet 4 Drive    21.4   6  258 110 3.08 3.215 19.44  1  0    3    1
#> Datsun 710        22.8   4  108  93 3.85 2.320 18.61  1  1    4    1

Created on 2022-08-02 by the reprex package (v2.0.1)

nathaneastwood commented 2 years ago

Looks good to me. One quick thing - could you squash your commits? Then I'll do a rebase and merge to keep the commit history clean.

etiennebacher commented 2 years ago

sorry @nathaneastwood I never did that before and I'm lost with all the git commands. I saw that you can merge and squash the commits automatically right? Out of curiosity, why do you want to squash commits of PRs?

nathaneastwood commented 2 years ago

No problem @etiennebacher :) I will do a squash and merge from here. Generally squashing the commits just keeps the commit history clean and you can keep commits related to a single change together in a single commit. I prefer to rebase and merge because then you don't get the "Merge commit" commits which make the commit history a little messier.

nathaneastwood commented 2 years ago

Ok that's interesting, the squash and merge also didn't include the "Merge commit" commit. Good to know! Thanks for the PR :D

etiennebacher commented 2 years ago

Thanks for the explanation ;) it's the first time I see someone requiring this in PRs but it's probably a better practice

nathaneastwood commented 2 years ago

It's more me just being pedantic :D