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

feat: add missing functions and docs for LazyFrame #31

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

etiennebacher
Copy link
Contributor

No description provided.

@etiennebacher
Copy link
Contributor Author

@eitsupi I start testing those functions and I find it a bit hard to read this kind of tests:

expect_query_equal(
    .input$select("int32"),
    .data,
    pl$DataFrame(int32 = 1:5)
  )

I think it's a good idea to test LazyFrame and DataFrame functions at once instead of manually copying tests, but the function above cannot take multiple datasets when testing joins for example. In tidypolars I have a simple script that copies test files for DataFrame and replace all occurrences of DataFrame by LazyFrame and more: https://github.com/etiennebacher/tidypolars/blob/main/tests/testthat/setup.R

This way, I can just write tests for DataFrame and it automatically generates the same tests for LazyFrame. It's also easier to see if an error only happens for eager/lazy data since tests appear in different files. What do you think about this approach?

@eitsupi
Copy link
Owner

eitsupi commented Nov 16, 2024

Sorry I did not notice the mention.

What do you think about this approach?

I think it's a good idea, but it may be difficult given the support for tests for things like head that behave differently in LazyFrame and DataFrame?

I'm also concerned about the possibility of files being updated on CI if run as part of R CMD check like setup.R, but I think that's a minor issue.

the function above cannot take multiple datasets when testing joins for example.

Could this be addressed by updating the function?

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.

2 participants