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

[codegen] Struct / enum derive PartialEq should also derive Eq #965

Closed
billy1624 opened this issue Aug 12, 2022 · 13 comments · Fixed by #988
Closed

[codegen] Struct / enum derive PartialEq should also derive Eq #965

billy1624 opened this issue Aug 12, 2022 · 13 comments · Fixed by #988
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@billy1624
Copy link
Member

Motivation

New clippy lint was added in Rust 1.63.0. which checks for types that derive PartialEq and could implement Eq.

Example:

#[derive(PartialEq)]
struct Foo {
    i_am_eq: i32,
    i_am_eq_too: Vec<String>,
}

Do this instead:

#[derive(PartialEq, Eq)]
struct Foo {
    i_am_eq: i32,
    i_am_eq_too: Vec<String>,
}

Proposed Solutions

Check sea-orm-codegen where #[derive(PartialEq)] should be changed to #[derive(PartialEq, Eq)].

Additional Information

Clippy lints index: https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq

@billy1624 billy1624 changed the title [codegen] [codegen] Struct / enum derive PartialEq should also derive Eq Aug 12, 2022
@billy1624 billy1624 added the good first issue Good for newcomers label Aug 12, 2022
@billy1624
Copy link
Member Author

Thanks! @baoyachi

@baoyachi
Copy link
Contributor

Oh,I see you've done it。

@billy1624
Copy link
Member Author

Nope, it's not

@billy1624
Copy link
Member Author

billy1624 commented Aug 12, 2022

#967 doesn't touch code generator of codegen

@baoyachi
Copy link
Contributor

It's seem's touch sea-query create

...
/// Value variants
///
/// We want Value to be exactly 1 pointer sized, so anything larger should be boxed.
+ #[derive(Clone, Debug, PartialEq)]
pub enum Value {
    Bool(Option<bool>),
    TinyInt(Option<i8>),
    SmallInt(Option<i16>),
    Int(Option<i32>),
...

@baoyachi
Copy link
Contributor

So,first need change sea-query?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 13, 2022

I don't think we need an option for this behaviour. It seems to be universally true that we want Eq and PartialEq wherever possible.

We can simply change the codegen for enum. For any struct (that contains a f64), implementing Eq is not possible.

@billy1624
Copy link
Member Author

There are only three place in the codegen we need to modify:

  • #[derive(Debug, Clone, PartialEq, EnumIter, DeriveActiveEnum #copy_derive #extra_derive)]
    #[sea_orm(rs_type = "String", db_type = "Enum", enum_name = #enum_name)]
    pub enum #enum_iden {
    #(
    #[sea_orm(string_value = #values)]
    #variants,
    )*
    }
    }
    }
  • #[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel #extra_derive)]
    pub struct Model {
    #(pub #column_names_snake_case: #column_rs_types,)*
    }
  • #[derive(Clone, Debug, PartialEq, DeriveEntityModel #extra_derive)]
    #[sea_orm(
    #schema_name
    table_name = #table_name
    )]
    pub struct Model {
    #(
    #attrs
    pub #column_names_snake_case: #column_rs_types,
    )*
    }

Btw... both f32 and f64 does not implement Eq but implemented PartialEq

@w93163red
Copy link
Contributor

May I take this one? If I understand correctly, only the above files need to be changed?

@billy1624 billy1624 added this to the 0.10.x milestone Aug 24, 2022
@billy1624
Copy link
Member Author

Hey @w93163red, sure! Please go ahead :)

@billy1624
Copy link
Member Author

billy1624 commented Aug 24, 2022

But note that if we have any f32 / f64 fields in the model, it shouldn't derive Eq (PartialEq is allowed). Also, we need to add a test cases for this as well.

@w93163red
Copy link
Contributor

I submitted a PR. If you find anything wrong, please let me know. Thanks~

@billy1624
Copy link
Member Author

Thanks! @w93163red Please check the comment: #988 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Archived in project
4 participants