adds doc tests to Godot Ext #139

astrale-sharp · 2023-03-05T13:21:47Z

adds doc to godot::log::*
adds passing doc test for bind.rs

lilizoey

Good to get some more missing documentation!
And having more info on how to use GodotExt is nice, doctests are nice but i am so bad at figuring out how to write them properly lol.

lilizoey · 2023-03-05T16:11:59Z

godot-core/src/bind.rs

+///
+///
+/// Note that you have to implement init otherwise you won't be able to call new or any
+/// other methods from gdscript


Missing period here, and some other places too.

lilizoey · 2023-03-05T16:16:03Z

godot-core/src/log.rs

@@ -4,6 +4,7 @@
 * file, You can obtain one at https://mozilla.org/MPL/2.0/.
 */

+/// Throws a godot warning


For this and the others, as they are equivalent to some godot-functions, you could add a

/// _Godot equivalent: @GlobalScope.push_warning()_

at the bottom. We do this in some other places too to make it easier for people familiar with godot.

lilizoey · 2023-03-05T16:21:52Z

godot-core/src/log.rs

@@ -75,6 +78,7 @@ pub use crate::{godot_error, godot_print, godot_script_error, godot_warn};
 use crate::builtin::{StringName, Variant};
 use crate::sys::{self, GodotFfi};

+/// prints to the godot console, it's use is to be used by the godot_print! macro


The wording is a little clunky here, maybe just drop "it's use is to be" and have it just say "used by the godot_print! macro".
Also it should be "its use" not "it's use".
I think you can also link to the godot_print macro in the doc here?
Also, missing capitalization and period.

yes that's better, thanks I'm not a native english speaker so this is a big help.

Bromeon

Thank you for this pull request 🙂

Maybe some general suggestions:

Please pay attention to use full English sentences even in documentation. That means: capitalization, full stops or punctuation in general.
For code examples, make sure they are properly indented and formatted (rustfmt).
In godot-rust, you can typically not use ``` in doctests, because the Godot engine is needed for a lot of things to work. Instead, use ```no_run.
Run ./check.sh locally to make sure some of the basic checks pass. See Contributing.md.
Pay attention to details like:
- surrounding library symbols with single backticks whenever you refer to them from docs
- capitalizing names properly ("GDScript" and "Godot", not "gdscript" and "godot")

Bromeon · 2023-03-05T16:33:13Z

godot-core/src/bind.rs

+///
+/// # Examples
+///
+/// ## Basic Reference


It's called RefCounted now (Reference in Godot 3).

And what do you mean with "basic"? Maybe be explicit and say:

/// ## With `RefCounted` as a base class

Bromeon · 2023-03-05T16:44:50Z

godot-core/src/log.rs

@@ -21,6 +22,7 @@ macro_rules! godot_warn {
    };
 }

+/// Throws a godot error


This doesn't throw an error (what does that even mean, there are no exceptions in Rust or GDScript?).
Instead, it prints one. Here, it would be interesting to see how it differentiates from godot_print and godot_warn.

The upstream Godot documentation can also be a good source of information, although in this particular case, the method doesn't exist as is.

lilizoey · 2023-03-05T16:50:17Z

Run ./check.sh locally to make sure some of the basic checks pass. See Contributing.md.

Running it locally for me didn't cause any errors, did it for you?

Bromeon · 2023-03-05T17:52:33Z

Running it locally for me didn't cause any errors, did it for you?

I didn't try, but I'd expect it to fail with doctests that need engine support.
Let's see what bors thinks.

bors try

bors · 2023-03-05T18:03:57Z

try

Build succeeded:

full-ci

astrale-sharp · 2023-03-07T08:37:00Z

Thank you for all the comments and pointers!

i ran check.sh before pushing and it passed but bors try seems to have some fail checks :/

astrale-sharp · 2023-03-07T08:47:28Z

I initially thought about ```no_run` but seeing as cargo test --doc was passing I decided to leave it off, should I add it ?

Bromeon · 2023-03-07T08:57:00Z

That's strange, it could mean that your doctests are not actually run.
Could you check if they are listed when you run cargo test or cargo test --doc?

astrale-sharp · 2023-03-07T08:59:22Z

They are listed for sure cause they didn't run initially (i forgot a bracket somewhere) and then they started passing.

astrale-sharp · 2023-03-07T09:00:29Z

I'm fixing some little things before i push on this PR again, taken comments into account and testing that what I write is actually correct.

astrale-sharp · 2023-03-07T09:09:29Z

I'm running cargo test --doc just to be sure

test src/bind.rs - bind::GodotExt (line 65) ... ok
test src/bind.rs - bind::GodotExt (line 34) ... ok

definitely running and passing.

just let me reread myself

astrale-sharp · 2023-03-07T09:14:29Z

Okay, took all comments into account, ran check.sh successfully again, I'm ready for another review or merging !

Bromeon

As already mentioned by sayaks on Discord, this will soon be obsolete, as #136 is almost ready.

I don't think it makes sense to merge this PR now, as it would just introduce conflicts for #136 and would be immediately reverted (for example, bind.rs is being removed by that PR). Instead, I would suggest:

to move the GodotExt documentation you added to the #[derive(GodotClass)] macro documentation, in godot-macros/src/lib.rs
to update it according to the new API in Virtual function dispatch #136 (should be relatively simple, GodotExt becomes <Base>Virtual, e.g. RefCountedVirtual)
to leave the print docs as-is, they are fine.

Bromeon · 2023-03-07T09:11:00Z

godot-core/src/bind.rs

+///     fn init(base : Base<Node>) -> Self {
+///         MyNode {base}
+///     }


Maybe run the examples through rustfmt:

Suggested change

/// fn init(base : Base<Node>) -> Self {

/// MyNode {base}

/// }

/// fn init(base: Base<Node>) -> Self {

/// MyNode { base }

/// }

running cargo fmt or rustfmt godot-core/src/bind.rs changes no file on my system, is that expected behaviour? I don't like to format by hand, my eyes are bad at this x)

it probably doesn't format doc test i assume ...

there's an easy workaround, you remove /// from each line of your doctests and let cargo fmt do the work

Bromeon · 2023-03-07T09:13:40Z

godot-core/src/bind.rs

+/// #[godot_api]
+/// impl MyRef {
+/// #[func]
+/// pub fn hello_world(&mut self) {
+///      godot_print!("Hello World!")
+///    }
+/// }


astrale-sharp · 2023-03-07T09:38:28Z

Is it better to wait for #136 to be merged, rebase and then move the documentation to macros/src/lib.rs?

Bromeon · 2023-03-07T09:49:18Z

#136 modifies neither macros/src/lib.rs nor the prints, so should be safe to do in parallel already.

Also, please squash commits to one once you're ready 🙂

astrale-sharp · 2023-03-07T09:54:19Z

One last question, the doc test are now failing (since NodeVirtual and RefCountedVirtual don't exist yet)
I think we should keep them failing until #136 is merged but i can also add no_run if you want.

I hope this was helping and I have not been annoying with my slow pace and multiple questions !

astrale-sharp · 2023-03-07T09:56:24Z

Apart from that, I squashed it good it should be good to go 😉

Bromeon · 2023-03-07T10:14:31Z

I think we should keep them failing until #136 is merged

Yep, agree, thanks a lot! This would also not need a rebase, bors automatically tests against latest master.

Bromeon · 2023-03-20T09:17:06Z

#136 is now merged, this has some conflicts.

@astrale-sharp could you rebase onto master?

adds doc tests to godot-macros

astrale-sharp · 2023-03-20T11:08:51Z

Oh yea, I removed docs from bind.rs which was surpressed

astrale-sharp · 2023-03-20T11:12:46Z

Seems to fail but I'm on my small laptop for weeks and it takes so long to compile godot-core x)

Bromeon · 2023-03-20T15:18:54Z

No worries, the failures were a network issue, nothing caused by you.

Thanks a lot for the documentation!
bors r+

bors · 2023-03-20T15:36:03Z

Build succeeded:

full-ci

lilizoey reviewed Mar 5, 2023

View reviewed changes

Bromeon reviewed Mar 5, 2023

View reviewed changes

bors bot added a commit that referenced this pull request Mar 5, 2023

Try #139:

b8497b4

Bromeon added the documentation Improvements or additions to documentation label Mar 7, 2023

astrale-sharp force-pushed the master branch from ceda0d2 to 8e50a46 Compare March 7, 2023 09:05

astrale-sharp force-pushed the master branch from 8e50a46 to 08a2880 Compare March 7, 2023 09:12

Bromeon reviewed Mar 7, 2023

View reviewed changes

astrale-sharp force-pushed the master branch from 4ce054b to e00858d Compare March 7, 2023 09:55

adds doc for log.rs

d8b3efb

adds doc tests to godot-macros

astrale-sharp force-pushed the master branch from e00858d to d8b3efb Compare March 20, 2023 11:07

bors bot merged commit cccf247 into godot-rust:master Mar 20, 2023

Bromeon mentioned this pull request Mar 29, 2023

Documentation for often-used APIs #35

Closed

adds doc tests to Godot Ext #139

adds doc tests to Godot Ext #139

Conversation

astrale-sharp commented Mar 5, 2023

lilizoey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey commented Mar 5, 2023 • edited Loading

Bromeon commented Mar 5, 2023

bors bot commented Mar 5, 2023

try

astrale-sharp commented Mar 7, 2023

astrale-sharp commented Mar 7, 2023

Bromeon commented Mar 7, 2023 • edited Loading

astrale-sharp commented Mar 7, 2023

astrale-sharp commented Mar 7, 2023

astrale-sharp commented Mar 7, 2023

astrale-sharp commented Mar 7, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrale-sharp commented Mar 7, 2023

Bromeon commented Mar 7, 2023 • edited Loading

astrale-sharp commented Mar 7, 2023

astrale-sharp commented Mar 7, 2023

Bromeon commented Mar 7, 2023

Bromeon commented Mar 20, 2023

astrale-sharp commented Mar 20, 2023

astrale-sharp commented Mar 20, 2023

Bromeon commented Mar 20, 2023

bors bot commented Mar 20, 2023

lilizoey commented Mar 5, 2023 •

edited

Loading

Bromeon commented Mar 7, 2023 •

edited

Loading

Bromeon left a comment •

edited

Loading

Bromeon commented Mar 7, 2023 •

edited

Loading