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 $with_columns() and $with_columns_seq() for DataFrame #78

Merged
merged 43 commits into from
Feb 14, 2025

Conversation

etiennebacher
Copy link
Contributor

@etiennebacher etiennebacher commented Feb 14, 2025

Sorry for the large amount of commits, most of them stem from #49 but rebasing gives a million conflicts.

This should be small enough to review without checking commits though.

Copy link
Owner

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I suspect now that we have the LazyFrame and DataFrame methods (with_columns() and with_columns_seq()), it would be a good time to add some tests.
Any thoughts on adding tests?

@etiennebacher
Copy link
Contributor Author

You mean adding tests for those two in this PR as well or in a separate one?

@eitsupi
Copy link
Owner

eitsupi commented Feb 14, 2025

I think it would be better if the relevant tests were added in this PR.
Ideally, it would be tested in CI (of course, right now there is no tests for PRs because it is far from ideal, but it will be tested on the R-universe once merged).

Of course if you want to improve the overall test you were previously working on first, you can add the test afterwards.

@etiennebacher
Copy link
Contributor Author

I'll add some tests here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improvements that were made here: https://github.com/eitsupi/neo-r-polars/pull/31/files#diff-62a08fcd787a5161df91277268cfbae6e2515c1cde86e968161a362fd2982164

Not strictly needed for this PR, mostly useful when there are several inputs (e.g. for joins) but I thought it would be ok to add it here since I start working on tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we still ok to store all "frame" tests here and have the expect_query_equal() test for both lazy and eager?

Some methods might be eager-only or lazy-only so it's still good to keep a "test-dataframe-frame" I think.

@eitsupi eitsupi changed the title feat: Add $with_columns() and $with_columns_seq() for DataFrame feat: add $with_columns() and $with_columns_seq() for DataFrame Feb 14, 2025
Copy link
Owner

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@eitsupi eitsupi merged commit b6c082f into eitsupi:main Feb 14, 2025
@eitsupi
Copy link
Owner

eitsupi commented Feb 14, 2025

(I didn't think too much about it, but as you know, the upstream polars repo require PR titles to start with a uppercase letter, so perhaps it would be better to follow that rule?)

@etiennebacher
Copy link
Contributor Author

etiennebacher commented Feb 14, 2025

Sure, I don't mind that. I'm just thinking that when we merge this to r-polars it will be one big squashed commit, right? Or will it be another approach? If it's one big squashed commit then the PR titles here don't really matter (although it's a good habit to have)

@eitsupi
Copy link
Owner

eitsupi commented Feb 14, 2025

I think it would be better for the future to keep the commit history of this repository (i.e. currently the next branch of r-polars) (that's why I remove the PR number every time I squash-merge PRs in this repository).

I do not think merging into the current r-polars main branch is worthwhile as there is too little in the codebase shared, and I think a strategy of renaming branches and switching default branches would be better (see examples such as docker compose v1 and v2).

@etiennebacher
Copy link
Contributor Author

I think a strategy of renaming branches and switching default branches would be better

Makes sense, I forgot about this approach, I think it's a good idea.

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