Add serde support for GString, StringName, NodePath and Array

[kuruk-mm]

I used godot-rust/gdnative#743 as a base PR, so thanks @lilizoey

I created some itest for testing these types.

You can use locally with:

./check.sh itest --use-serde -f serde_roundtrip

For executing tests, the CI needs to add the serde (not godot/serde) to the features on the itest crate. If there is a better solution for this, please, let me know.

[GodotRust]

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-508

[Bromeon]

Thanks a lot, great addition! 😊

I don't think the core should depend on a particular serde format, but rather just provide the traits. Could you remove the serde_json dependency from godot-core? It can stay in itest.

From a file organization point of view, I think it could make sense to have a dedicated serde_test.rs with all the new test cases. Then we'd only need one #[cfg] on the outer module.

[kuruk-mm]

Thanks a lot, great addition! 😊

More than happy to contribute :)

I don't think the core should depend on a particular serde format, but rather just provide the traits. Could you remove the serde_json dependency from godot-core? It can stay in itest.

godot-core should depends on serde_json (only serde feature) because it is being used here:

https://github.com/kuruk-mm/gdext/blob/d30fc8431e06ae5acda5a63af0604de774b117ae/godot-core/src/builtin/mod.rs#L148-L165

The CI is not executing the serde feature, so for that reason, it's not giving an error.

If I remove I get:

error[E0433]: failed to resolve: use of undeclared crate or module `serde_json`
   --> godot-core/src/builtin/mod.rs:156:28
    |
156 |         let json: String = serde_json::to_string(value).unwrap();
    |                            ^^^^^^^^^^ use of undeclared crate or module `serde_json`

error[E0433]: failed to resolve: use of undeclared crate or module `serde_json`
   --> godot-core/src/builtin/mod.rs:157:23
    |
157 |         let back: T = serde_json::from_str(json.as_str()).unwrap();
    |                       ^^^^^^^^^^ use of undeclared crate or module `serde_json`

For more information about this error, try `rustc --explain E0433`.
error: could not compile `godot-core` (lib test) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

[Bromeon]

godot-core should depends on serde_json (only serde feature) because it is being used here:

But where is test_utils::roundtrip even used? Can you not move it into the itest crate?

Dependencies in godot-core affect every single user and all CI jobs, so we really have to be very careful to add them only when absolutely necessary. Short compile times are one of gdext's top priorities -- also a reason why we avoid syn and why serde is feature-gated in the first place 🙂

Btw, it looks like there's an issue with GitHub actions, your last commit didn't trigger the pipeline for some reason. Might be a downtime...

[kuruk-mm]

fect every single user and all CI jobs, so we really have to be very careful to add them only when absolutely necessary. Short compile times are one of gdext's top priorities -- also a reason why we avoid syn and why serde is feature-gated in the first place 🙂

It's used in tests (not itest). Example: https://github.com/kuruk-mm/gdext/blob/master/godot-core/src/builtin/aabb.rs#L424-L431
It was previously implemented like that. In the main branch, if you use cargo test --features serde, you're going to get the error.

I moved the serde_json to the dev-dependencies. Looks like I cannot use features in dev-dependencies 🤔

I have just push-force the code with all you're reviews fixed.

Edit: Last force push, I added license header for serde_tests.rs

[Bromeon]

It's used in tests (not itest). Example: https://github.com/kuruk-mm/gdext/blob/master/godot-core/src/builtin/aabb.rs#L424-L431
It was previously implemented like that. In the main branch, if you use cargo test --features serde, you're going to get the error.

Oh, thanks a lot! It looks like CI doesn't cover that properly. [dev-dependencies] is indeed better because users don't typically have to pull and compile those. I'll look into this more closely once this PR is merged.

[Bromeon]

Thank you 🙂