Add support for custom operators #641
Conversation
|
Some questions:
EDIT: it appears we don't need |
There was a problem hiding this comment.
Hey @rakbladsvalsen, thanks for the contribution!! I got a few comments :)
| pub(crate) fn get_bin_oper(&self) -> Option<BinOper> { | ||
| pub(crate) fn get_bin_oper(&self) -> Option<&BinOper> { | ||
| match self { | ||
| Self::Binary(_, oper, _) => Some(*oper), | ||
| Self::Binary(_, oper, _) => Some(oper), |
src/types.rs
Outdated
| @@ -72,6 +72,8 @@ impl Clone for SeaRc<dyn Iden> { | |||
| } | |||
| } | |||
|
|
|||
| impl Eq for SeaRc<dyn Iden> {} | |||
There was a problem hiding this comment.
Hmmmm... Eq for dyn Iden
Consider this. Where both dyn Iden yield the same identifier id but it's not the same Rust structure / enum.
#[derive(Iden)]
struct Id;
// Both yield the string "id"
assert_eq!(Id.to_string(), "id");
assert_eq!(Alias::new("id").to_string(), "id");
// How about this??
assert_eq!(Id, Alias::new("id"));There was a problem hiding this comment.
Isn't this already covered by the PartialEq implementation that already exists for DynIden? The current implementation checks two things: whether the .to_string() contents are equal and whether the vtable pointer is the same for the two objects, though this only works for dyn objects.
Anyway, if we wanted to provide the ability to do something like the last line in your example, it'd be necessary to implement something like fn get_iden_id() to allow direct comparison between Iden objects, i.e. Id.get_iden_id() == Alias::new("id").get_iden_id() (which is probably already doable using to_string()).
Though I must say, this feels rather unnatural. I don't think comparison between two different struct types is something a lot of users are doing. There are no feature requests for this either, so I think we're good in that regard.
src/types.rs
Outdated
| @@ -158,7 +160,7 @@ pub enum UnOper { | |||
| } | |||
|
|
|||
| /// Binary operator | |||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |||
| #[derive(Debug, Clone, PartialEq, Eq)] | |||
There was a problem hiding this comment.
We should not remove the Copy attribute.
src/types.rs
Outdated
| @@ -185,6 +187,7 @@ pub enum BinOper { | |||
| RShift, | |||
| As, | |||
| Escape, | |||
| Custom(DynIden), | |||
There was a problem hiding this comment.
May be we can have a new type CustomOper, which is simply a wrapped &'static str.
This would have avoided removing Copy.
There was a problem hiding this comment.
Right, I was not aware we need to remove the Copy derive. So, yes, &'static str is a better alternative.
@rakbladsvalsen what do you think?
There was a problem hiding this comment.
Thanks for the feedback guys! This PR should be good to go.
🎉 Released In 0.29.1 🎉Thank you everyone for the contribution! |
PR Info
See SeaORM discord
New Features
Regexop for PostgresRegexCaseInsensitiveop for PostgresNotes
This PR is still a little bit rough around the edges, but hey, I'm open to comments. :)