-
-
Notifications
You must be signed in to change notification settings - Fork 523
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
Conversation
Add code examples for the macros
Add code examples for the macros
Wow! Huge thanks for this!! I want to do this for long time but busy with feature release... loll |
Glad I could do it. |
I think for all of our important traits and derive macros, we could document it this way...
P.S. this is what I did on #258. Any thoughts? @tyt2y3 @charleschege |
Btw... you could use |
@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? |
Sounds good |
Why did the build fail? |
Provide module level code example on how to create an Entity, Model, ActiveModel, Column and PrimaryKey
@billy1624 know why the build failed? |
@tyt2y3 I am investigating a way to solve the issue. What I have discovered on my end is that the Github workflow runs Somehow the test is not detecting crate imports in the documentation. There are errors like
even though |
Which error? There are a lot of errors loll |
@billy1624 @tyt2y3 Can someone help me with this issue? Running But when i run |
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! |
Is this related? #249 |
Hey @charleschege, is it possible for you to grant write access to us? I want to add the follow code snippet to 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 }} |
I have added access to the repo |
My issues stem from this test run: |
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] |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 ActiveValue
and a code snippet in the documentation. Take a look at commit 30814f0
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
ActiveValue
and a code snippet in the documentation. Take a look at commit 30814f0
Thank you
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
p�[200pub struct ActiveValue<V> Add a code snippet for a simple example
[dev-dependencies] | ||
sea-orm = { path = "../", features = ["macros"] } | ||
serde = { version = "^1.0", features = ["derive"] } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
wow now it builds! a big thanks to @billy1624 |
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 }} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[dev-dependencies] | ||
sea-orm = { path = "../", features = ["macros"] } | ||
serde = { version = "^1.0", features = ["derive"] } |
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Adds documentation for all public types for this crate.