Add serde support for GString, StringName, NodePath and Array by kuruk-mm · Pull Request #508 · godot-rust/gdext (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation14 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

Choose a reason for hiding this comment

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

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.

check.sh Outdated Show resolved Hide resolved

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...

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...

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

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.

Choose a reason for hiding this comment

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

Thank you 🙂