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

Documentation for sea-orm #280

Merged
merged 21 commits into from
Nov 1, 2021
Merged

Documentation for sea-orm #280

merged 21 commits into from
Nov 1, 2021

Conversation

charleschege
Copy link
Contributor

Adds documentation for all public types for this crate.

@billy1624
Copy link
Member

Wow! Huge thanks for this!! I want to do this for long time but busy with feature release... loll

@charleschege
Copy link
Contributor Author

Glad I could do it.

@billy1624
Copy link
Member

I think for all of our important traits and derive macros, we could document it this way...

  1. Describe what this trait does and its usage. How to derive it manually? Is there any derive macros for it? How? If there is any.

    /// A Rust representation of enum defined in database.
    ///
    /// # Implementations
    ///
    /// You can implement [ActiveEnum] manually by hand or use the derive macro [DeriveActiveEnum](sea_orm_macros::DeriveActiveEnum).
    ///
    /// # Examples
    ///
    /// Implementing it manually versus using the derive macro [DeriveActiveEnum](sea_orm_macros::DeriveActiveEnum).
    ///
    /// > See [DeriveActiveEnum](sea_orm_macros::DeriveActiveEnum) for the full specification of macro attributes.
    ///
    /// ```rust
    /// use sea_orm::entity::prelude::*;
    ///
    /// // Using the derive macro
    /// #[derive(Debug, PartialEq, DeriveActiveEnum)]
    /// #[sea_orm(rs_type = "String", db_type = "String(Some(1))")]
    /// pub enum DeriveCategory {
    /// #[sea_orm(string_value = "B")]
    /// Big,
    /// #[sea_orm(string_value = "S")]
    /// Small,
    /// }
    ///
    /// // Implementing it manually
    /// #[derive(Debug, PartialEq)]
    /// pub enum Category {
    /// Big,
    /// Small,
    /// }
    ///
    /// impl ActiveEnum for Category {
    /// // The macro attribute `rs_type` is being pasted here
    /// type Value = String;
    ///
    /// // Will be atomically generated by `DeriveActiveEnum`
    /// fn to_value(&self) -> Self::Value {
    /// match self {
    /// Self::Big => "B",
    /// Self::Small => "S",
    /// }
    /// .to_owned()
    /// }
    ///
    /// // Will be atomically generated by `DeriveActiveEnum`
    /// fn try_from_value(v: &Self::Value) -> Result<Self, DbErr> {
    /// match v.as_ref() {
    /// "B" => Ok(Self::Big),
    /// "S" => Ok(Self::Small),
    /// _ => Err(DbErr::Type(format!(
    /// "unexpected value for Category enum: {}",
    /// v
    /// ))),
    /// }
    /// }
    ///
    /// fn db_type() -> ColumnDef {
    /// // The macro attribute `db_type` is being pasted here
    /// ColumnType::String(Some(1)).def()
    /// }
    /// }
    /// ```
    ///
    /// Using [ActiveEnum] on Model.
    ///
    /// ```
    /// use sea_orm::entity::prelude::*;
    ///
    /// // Define the `Category` active enum
    /// #[derive(Debug, Clone, PartialEq, DeriveActiveEnum)]
    /// #[sea_orm(rs_type = "String", db_type = "String(Some(1))")]
    /// pub enum Category {
    /// #[sea_orm(string_value = "B")]
    /// Big,
    /// #[sea_orm(string_value = "S")]
    /// Small,
    /// }
    ///
    /// #[derive(Clone, Debug, PartialEq, DeriveEntityModel)]
    /// #[sea_orm(table_name = "active_enum")]
    /// pub struct Model {
    /// #[sea_orm(primary_key)]
    /// pub id: i32,
    /// // Represents a db column using `Category` active enum
    /// pub category: Category,
    /// pub category_opt: Option<Category>,
    /// }
    ///
    /// #[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
    /// pub enum Relation {}
    ///
    /// impl ActiveModelBehavior for ActiveModel {}
    /// ```
    pub trait ActiveEnum: Sized {
    /// Define the Rust type that each enum variant represents.
    type Value: Into<Value> ValueType Nullable TryGetable;
    /// Convert enum variant into the corresponding value.
    fn to_value(&self) -> Self::Value;
    /// Try to convert the corresponding value into enum variant.
    fn try_from_value(v: &Self::Value) -> Result<Self, DbErr>;
    /// Get the database column definition of this active enum.
    fn db_type() -> ColumnDef;
    }

  2. Document the derive macros and its macro attributes

    /// A derive macro to implement `sea_orm::ActiveEnum` trait for enums.
    ///
    /// # Limitations
    ///
    /// This derive macros can only be used on enums.
    ///
    /// # Macro Attributes
    ///
    /// All macro attributes listed below have to be annotated in the form of `#[sea_orm(attr = value)]`.
    ///
    /// - For enum
    /// - `rs_type`: Define `ActiveEnum::Value`
    /// - Possible values: `String`, `i8`, `i16`, `i32`, `i64`, `u8`, `u16`, `u32`, `u64`
    /// - Note that value has to be passed as string, i.e. `rs_type = "i8"`
    /// - `db_type`: Define `ColumnType` returned by `ActiveEnum::db_type()`
    /// - Possible values: all available enum variants of `ColumnType`, e.g. `String(None)`, `String(Some(1))`, `Integer`
    /// - Note that value has to be passed as string, i.e. `db_type = "Integer"`
    ///
    /// - For enum variant
    /// - `string_value` or `num_value`:
    /// - For `string_value`, value should be passed as string, i.e. `string_value = "A"`
    /// - For `num_value`, value should be passed as integer, i.e. `num_value = 1` or `num_value = 1i32`
    /// - Note that only one of it can be specified, and all variants of an enum have to annotate with the same `*_value` macro attribute
    #[proc_macro_derive(DeriveActiveEnum, attributes(sea_orm))]
    pub fn derive_active_enum(input: TokenStream) -> TokenStream {
    let input = parse_macro_input!(input as DeriveInput);
    match derives::expand_derive_active_enum(input) {
    Ok(ts) => ts.into(),
    Err(e) => e.to_compile_error().into(),
    }
    }

P.S. this is what I did on #258.

Any thoughts? @tyt2y3 @charleschege

@billy1624
Copy link
Member

Btw... you could use cargo nightly fmt --all to format the doc comments. See #121.

@charleschege
Copy link
Contributor Author

charleschege commented Oct 29, 2021

@billy1624 would adding the documentation examples for using derive macros and traits manually duplicate the examples? Can we use module level documentation for this to show how to use both manual implementations of a trait and derives of the trait, then we refer them from the inline documentation?

@billy1624
Copy link
Member

Sounds good

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 29, 2021

Why did the build fail?

Provide module level code example on how to create an Entity, Model, ActiveModel, Column and PrimaryKey
@tyt2y3
Copy link
Member

tyt2y3 commented Oct 30, 2021

@billy1624 know why the build failed?

@charleschege
Copy link
Contributor Author

@tyt2y3 I am investigating a way to solve the issue. What I have discovered on my end is that the Github workflow runs cargo test --workspace which tests even the documentation.

Somehow the test is not detecting crate imports in the documentation. There are errors like

  |
3 | use sea_orm::entity::prelude::*;
  |     ^^^^^^^ use of undeclared crate or module `sea_orm`

even though use sea_orm::entity::prelude::*; is imported.

@billy1624
Copy link
Member

@billy1624 know why the build failed?

Which error? There are a lot of errors loll

@charleschege
Copy link
Contributor Author

@billy1624 @tyt2y3 Can someone help me with this issue?

Running cargo test --workspace seems to be the issue. It is not detecting sea_orm_macros in documentation examples.

But when i run cargo test --doc or cargo test --all-targets everything works fine, the macros and modules are detected. Does anyone have suggestions on how to import macros on documentation tests ?

@billy1624
Copy link
Member

Hey @charleschege, can you point out which error should I look into? E.g. https://github.com/SeaQL/sea-orm/runs/4054619075?check_suite_focus=true#step:4:589 this error. Thanks!

@billy1624
Copy link
Member

Click on the line number on the left and copy & paste the URL

image

@billy1624
Copy link
Member

@billy1624 @tyt2y3 Can someone help me with this issue?

Running cargo test --workspace seems to be the issue. It is not detecting sea_orm_macros in documentation examples.

But when i run cargo test --doc or cargo test --all-targets everything works fine, the macros and modules are detected. Does anyone have suggestions on how to import macros on documentation tests ?

Is this related? #249

@billy1624
Copy link
Member

billy1624 commented Oct 30, 2021

Hey @charleschege, is it possible for you to grant write access to us? I want to add the follow code snippet to .github/workflows/rust.yml.

  test-temp:
    name: Unit Test (Temp)
    runs-on: ubuntu-20.04
    strategy:
      matrix:
        args: ["--workspace", "--doc", "--all-targets"]
      fail-fast: false
    steps:
      - uses: actions/checkout@v2

      - uses: actions-rs/toolchain@v1
        with:
          profile: minimal
          toolchain: stable
          override: true

      - uses: actions-rs/cargo@v1
        with:
          command: test
          args: ${{ matrix.args }}

https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@charleschege
Copy link
Contributor Author

I have added access to the repo

@charleschege
Copy link
Contributor Author

Hey @charleschege, can you point out which error should I look into? E.g. https://github.com/SeaQL/sea-orm/runs/4054619075?check_suite_focus=true#step:4:589 this error. Thanks!

My issues stem from this test run:
https://github.com/SeaQL/sea-orm/runs/4054619075?check_suite_focus=true

@billy1624
Copy link
Member

Hey @charleschege, it’s already 11pm (HKT) for me. I’m about to sleep. Will talk to you tmr.

@@ -22,6 24,7 @@ where
ActiveValue::set(v)
}

/// Defines an unset operation on an [ActiveValue]

Choose a reason for hiding this comment

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

Could some documentation be added covering why this takes an unused Option<bool>? I don't feel the type's purpose is very clear, even having dug through the source (off-topic for this PR, but would this perhaps be better served by either a 3-value enum or an Option of a 2-value enum? that would allow this to be more self-documenting)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documenting that is a little bit tricky since the field of ActiveValue are private. I think a user would have to build the documentation with --document-private-items set to see any inner struct documentation.

I have tried to provide an overview of the internals of ActiveValueand a code snippet in the documentation. Take a look at commit 30814f0

Copy link
Member

@tyt2y3 tyt2y3 Oct 31, 2021

Choose a reason for hiding this comment

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

Hi there, welcome! You have made a good point.

It certainly is trying to mock a enum, where Set & Unset is like Ok and Err. It could as well be Unset(()).

I was waiting for private enum variant rust-lang/rfcs#2028 (comment) to hide the Unchanged variant from public use.

And that's why.

Copy link
Member

Choose a reason for hiding this comment

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

I have tried to provide an overview of the internals of ActiveValueand a code snippet in the documentation. Take a look at commit 30814f0

Thank you

Copy link

@jam1garner jam1garner Oct 31, 2021

Choose a reason for hiding this comment

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

in the case of

fn Unset<V>(_: Option<bool>)
//          ^^^^^^^^^^^^^^^

Why the unused parameter? I understand that Unset is supposed to emulate a variant of an enum but that still doesn't explain to me why the parameter.

Copy link
Member

@tyt2y3 tyt2y3 Nov 1, 2021

Choose a reason for hiding this comment

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

Because Unset() is not a valid enum. I agree this is a little confusing. May be I should change it in 0.5 to something like

enum ActiveValue {
    Set(Value),
    UnchangedDoNotUse,
    NotSet,    
}

What do you think?

Choose a reason for hiding this comment

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

@tyt2y3 something like that would be a positive change, yes. Tbh if the argument has no meaning, the name Unset seems fine to me (albeit slightly ambiguous if you mean "unset" (verb, action, to switch to a no longer set state) or "unset" (adjective, state, not currently set)), I mostly think just getting rid of the argument is enough for it to make sense to me. Thanks!

charleschege and others added 3 commits October 31, 2021 09:03
Comment on lines 25 to 27
[dev-dependencies]
sea-orm = { path = "../", features = ["macros"] }
serde = { version = "^1.0", features = ["derive"] }
Copy link
Member

Choose a reason for hiding this comment

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

Hey @charleschege, I have fixed all the errors inside sea-orm doctest on fcf3ea9. And I know why cargo test --doc passes while cargo test --workspace fails.

In sea-orm-macros/src/lib.rs, you have include some code snippets that requires sea-orm. However, sea-orm-macros does not depends on sea-orm. So, all of the doctests failed. By adding dev-dependencies will solve the problem. 6904b9f

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This has resolved all the issues I had when testing documentation examples.

sea-orm-macros/src/lib.rs Show resolved Hide resolved
@tyt2y3
Copy link
Member

tyt2y3 commented Oct 31, 2021

wow now it builds! a big thanks to @billy1624

Comment on lines 263 to 282
test-temp:
name: Unit Test (Temp)
runs-on: ubuntu-20.04
strategy:
matrix:
args: ["--workspace", "--doc", "--all-targets"]
fail-fast: false
steps:
- uses: actions/checkout@v2

- uses: actions-rs/toolchain@v1
with:
profile: minimal
toolchain: stable
override: true

- uses: actions-rs/cargo@v1
with:
command: test
args: ${{ matrix.args }}
Copy link
Member

Choose a reason for hiding this comment

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

Just a kind reminder. Removing it before merge :P

Copy link
Contributor Author

@charleschege charleschege left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 25 to 27
[dev-dependencies]
sea-orm = { path = "../", features = ["macros"] }
serde = { version = "^1.0", features = ["derive"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This has resolved all the issues I had when testing documentation examples.

sea-orm-macros/src/lib.rs Show resolved Hide resolved
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you for the meticulous work

@tyt2y3 tyt2y3 merged commit d5da351 into SeaQL:master Nov 1, 2021
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.

4 participants