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

Add have any criteria #1042

Merged
merged 10 commits into from
May 17, 2024

Conversation

davidepaolotua
Copy link
Contributor

Implements the feature in #1030
However, the method was renamed to have_any?, instead of has_any, just because an array column will prolly be called in the plural form (e.g: Users.tags) and have_any just sounded better.

* Implement allow_nulls_for and forbid_nulls_for

* Remove leftover macro, fix ordering of change default with respect to change type
Comment on lines +92 to +112
it "changes defaults after changing the type" do
built = Avram::Migrator::AlterTableStatement.new(:users).build do
change_type id : Int64
change_default id : Int64, default: 54
end
built.statements.size.should eq 2
built.statements[0].should eq "ALTER TABLE users ALTER COLUMN id SET DATA TYPE bigint;"
built.statements[1].should eq "ALTER TABLE ONLY users ALTER COLUMN id SET DEFAULT '54';"
end

it "can change column nullability" do
built = Avram::Migrator::AlterTableStatement.new(:users).build do
forbid_nulls_for :id
allow_nulls_for :age
end

built.statements.size.should eq 2
built.statements[0].should eq "ALTER TABLE users ALTER COLUMN id SET NOT NULL;"
built.statements[1].should eq "ALTER TABLE users ALTER COLUMN age DROP NOT NULL;"
end

Copy link
Member

Choose a reason for hiding this comment

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

oops! Looks like this snuck in 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.

🤕 did the checkout from the wrong branch 😢

@@ -154,6 +154,11 @@ class Avram::Criteria(T, V)
add_clause(Avram::Where::In.new(column, values))
end

def have_any?(values) : T
Copy link
Member

Choose a reason for hiding this comment

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

I like the name better than my original idea; however, my only (very very small) concern is would this be confusing that it's a predicate method that doesn't return Bool or denote a nilable value? Maybe it's fine in this case, but I don't want people to confuse it with the none? or any? methods we have.

if UserQuery.new.tags.have_any?(["featured"])
  # would always return truethy
end

# would this be strange? lol
UserQuery.new.tags.have_any?(["featured"]).any?

But if you don't think it'll be that big of an issue with some proper docs, then I'm cool with it as is 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhmm... good point... especially the if branch I can see myself falling into it quite easily.
Maybe renaming it to "overlapping_with" would be clearer?

Copy link
Member

Choose a reason for hiding this comment

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

What about just have_any without the question mark?

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

nice!

@jwoertink jwoertink merged commit c12d506 into luckyframework:main May 17, 2024
8 of 9 checks passed
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