fix: Keeps Enum order when converting to R

[andyquinterom]

Closes #1251

[etiennebacher]

Hi, thanks for opening the issue and the corresponding PR, can you add a test and a bullet point in NEWS?

[andyquinterom]

Hi, thanks for opening the issue and the corresponding PR, can you add a test and a bullet point in NEWS?

I'll get on the tests asap

[andyquinterom]

@etiennebacher I added a test to validate many cases.

  # Generate 100 random data frames with random
  # orders for levels. They should always match
  for (i in 1:100) {
    expected_levels = sample(letters, length(letters))
    expected_values = sample(letters, length(letters))
    expect_identical(
      as_polars_series(expected_values)$
        cast(pl$Enum(expected_levels))$
        to_r(),
      factor(expected_values, levels = expected_levels)
    )
  }

[andyquinterom]

@etiennebacher Everything seems to pass except for R CMD check due to Non-api calls, but I think that's out of the scope of this pull request

[etiennebacher]

Everything seems to pass except for R CMD check due to Non-api calls, but I think that's out of the scope of this pull request

Yes this is out of scope. I agree with @eitsupi about using property-based testing on this. I think something like this would do the job (but I haven't tested yet):

test_that("reversing twice returns the original input", {
  for_all(
    values = any_atomic(any_na = TRUE),
	levels = any_atomic(any_na = TRUE),
    property = function(values, levels) {
	    expect_identical(
	      as_polars_series(values)$
	        cast(pl$Enum(levels))$
	        to_r(),
	      factor(values, levels = levels)
	    )
    }
  )
})

Please also add a bullet point in news with the PR number and your username (if you want).

[eitsupi]

Thanks for the updates!

Could you bump the Rust lib version and please remove the lib-sums.tsv.
Please check the document for details.
https://github.com/pola-rs/r-polars/blob/8ecb1a358c5bf0c6c6e6c9527904bb798f07a101/DEVELOPMENT.md

[andyquinterom]

Thanks for the updates!

Could you bump the Rust lib version and please remove the lib-sums.tsv. Please check the document for details. https://github.com/pola-rs/r-polars/blob/8ecb1a358c5bf0c6c6e6c9527904bb798f07a101/DEVELOPMENT.md

I just ran the task and see that nothing has changed. Am I doing something wrong?

[eitsupi]

We should update lib version by hand first.
https://github.com/pola-rs/r-polars/blob/main/src%2Frust%2FCargo.toml#L3-L3

[etiennebacher]

Thanks @andyquinterom

@eitsupi I can't merge, I need you to approve

[eitsupi]

Almost looks good.
A few comments.

[eitsupi]

How about including NA?

[eitsupi]

I tried but there is an issue about that.
armcn/quickcheck#22

[eitsupi]

Thanks!