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.

Sign up for GitHub

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

Jump to bottom

Allow for Between, Not, All, Cols in @select #372

Merged
merged 1 commit into from
Dec 22, 2023
Merged

Allow for Between, Not, All, Cols in @select #372

merged 1 commit into from
Dec 22, 2023

Conversation

pdeffebach
Copy link
Collaborator

No description provided.

@pdeffebach
Copy link
Collaborator Author

Tests fail because of @linq. This means we finally get to deprecate @linq completely and remove it!

@bkamins
Copy link
Member

bkamins commented Dec 20, 2023

I would remove @linq.

bkamins
bkamins approved these changes Dec 20, 2023
View reviewed changes
Copy link
Member

@bkamins bkamins left a comment

Choose a reason for hiding this comment

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

Maybe add a comment that anything within these calls is left untouched. What I mean is that Cols(:A) does not do re-interpretation of :A.

@pdeffebach
Copy link
Collaborator Author

Doctest failures are fixed in #367 so I'm comfortable merging.

@pdeffebach
Copy link
Collaborator Author

Actually, I'm going to change the implementation slightly so @groupby works better.

@pdeffebach
Copy link
Collaborator Author

@bkamins okay to merge? This is a pretty simple PR. Will open a new PR for @groupby using this shortly.

@bkamins
Copy link
Member

bkamins commented Dec 22, 2023

Looks good

@pdeffebach pdeffebach merged commit 8ebefa1 into master Dec 22, 2023
13 of 19 checks passed
@pdeffebach pdeffebach deleted the special_not branch December 22, 2023 17:31
@pdeffebach pdeffebach mentioned this pull request Dec 22, 2023
Merged
This was referenced Feb 26, 2024
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkamins bkamins bkamins approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pdeffebach @bkamins