-
Notifications
You must be signed in to change notification settings - Fork 276
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
[ci] #1658: Add documentation check #2367
[ci] #1658: Add documentation check #2367
Conversation
Codecov Report
@@ Coverage Diff @@
## iroha2-dev #2367 +/- ##
==============================================
+ Coverage 65.50% 67.02% +1.52%
==============================================
Files 133 140 +7
Lines 24697 26282 +1585
==============================================
+ Hits 16177 17616 +1439
- Misses 8520 8666 +146
Continue to review full report at Codecov.
|
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.
According to the CI report, a few comments need to be corrected to pass the CI checks.
84ff403
to
05fde26
Compare
@@ -30,7 +30,7 @@ pub enum FfiResult { | |||
Ok = 0_i32, | |||
} | |||
|
|||
/// Implement [`Handle`] for given types with first argument as the initial handle id. |
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.
@ales-tsurko this is the only link that is still unresolved.
I think my knowledge of Rust is limited here to understand why all the syntax I'm trying is failing, so if you have any ideas how to fix this -- please do
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.
ok, thanks!
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.
The problem caused by the Handle
trait defined inside macro, so there's no trait definition until the macro is used. We have two options:
- define the trait outside the macro;
- don't link documentation for the trait.
@appetrosyan 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.
maybe we can omit the link altogether? the description would still be clear even without the link
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.
don't link documentation for the trait.
Is fine by me. We should avoid defining non-private traits inside macros, though.
Signed-off-by: Ales Tsurko <ales.tsurko@gmail.com>
Signed-off-by: Ekaterina Mekhnetsova <mekkatya@gmail.com>
Signed-off-by: Ales Tsurko <ales.tsurko@gmail.com>
Signed-off-by: Ales Tsurko <ales.tsurko@gmail.com>
Signed-off-by: Ales Tsurko <ales.tsurko@gmail.com>
34a4919
to
c213d16
Compare
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
Co-authored-by: Ekaterina Mekhnetsova <mekkatya@gmail.com> Signed-off-by: Ales Tsurko <ales.tsurko@gmail.com> Signed-off-by: BAStos525 <jungle.vas@yandex.ru>
Sorry for omitting the PR template here, but the change is very little and clear. It just adds documentation check (using
cargo doc
) to CI.