Added reset_where method for specific columns. #325
Conversation
| @@ -135,6 +135,11 @@ class Avram::Criteria(T, V) | |||
| rows.query.group_by(column) | |||
| end | |||
|
|
|||
| # :nodoc: | |||
| def __reset_where : Avram::QueryBuilder | |||
There was a problem hiding this comment.
Is there a way to make this private?
It feels weird to have to use __ in a lang that has real method protection.
There was a problem hiding this comment.
I agree, it is something I would do in JS. The question is, should it be private? Things like Thread for example are considered private in Crystal, while they are accessible.
There was a problem hiding this comment.
It may be possible but would require reworking how Criteria and Queries work. This needs to be public so the Queryable can call it, but it shouldn't be used by the public. It can be and it'll work but it just reads strangely so adding the __ hides it a bit more.
We also do this in distinct_on so that you do MyQuery.new.distinct_on(&.name) isntead of MyQuery.new.name.distinct_on which reads super weird
There was a problem hiding this comment.
Hmm I'm not sure what you mean. If marked as private I am fairly certain it will not work since we are calling it from another class
I don't think there is a way to make this method only accessible from some classes, but not others. But maybe I'm misunderstanding
There was a problem hiding this comment.
No, you're right, there isn't another way. You can't make it private. But I think the __ can be omitted. Some methods and object in Crystal are considered private, while being publicly accessible. A mention in the docs would be sufficient, I think.
| @@ -135,6 +135,11 @@ class Avram::Criteria(T, V) | |||
| rows.query.group_by(column) | |||
| end | |||
|
|
|||
| # :nodoc: | |||
| def __reset_where : Avram::QueryBuilder | |||
There was a problem hiding this comment.
It may be possible but would require reworking how Criteria and Queries work. This needs to be public so the Queryable can call it, but it shouldn't be used by the public. It can be and it'll work but it just reads strangely so adding the __ hides it a bit more.
We also do this in distinct_on so that you do MyQuery.new.distinct_on(&.name) isntead of MyQuery.new.name.distinct_on which reads super weird
|
|
||
| query.reset_where(:age) | ||
|
|
||
| query.args.should be_empty |
There was a problem hiding this comment.
Could you add a statement test here to match what was done on line 41?
There was a problem hiding this comment.
That's what I mentioned on Gitter 😂 . The result of where_sql is cached, so the test would fail. You can't call statement twice because it will always return the first result.
There was a problem hiding this comment.
Looks good! Thanks @wout. If someone can figure out a better way to "hide" or make that method private and wants to do a PR I'll accept it. I can't thin of a way but if we can that'd be cool
You're welcome! Just a thought, maybe something like this would make it less awkward: def private_reset_where
...
endMakes it instantly obvious that this is not something for an end user. |
|
I like that! Once I merge this in could you create a PR changing |
|
Of course, coming up! |
This will reset where for a specific column. Initially, you could also call
query.reset_wherewithout a block, which would reset all where clauses. But I removed that because it could lead to unexpected/unwanted results.Closes #319