Derive skip

[astrale-sharp]

Adds variant(skip) To/FromVariant proc macro

[astrale-sharp]

Note that this is based upon my previous PR (which should obviously be merged first)

[astrale-sharp]

added missing godot::builtin::, sorry about that

[Bromeon]

There are merge conflicts, could you resolve them please, @astrale-sharp?

[GodotRust]

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

[Bromeon]

What are your plans with this PR?

CI is currently red, and there is stll the open question how we should deserialize a struct with a skipped field. What value should it get? Default::default()?

[astrale-sharp]

For me it should get Default::default() yes, unless a #[variant(default = ...)] is there.

Originally i was blocked by ./check.sh itest crashing but this seems to be resolved so I'm going to work on this !

[Bromeon]

It may be worth rebasing onto latest master, as we fixed a lot of FFI stuff lately.
Maybe also use the opportunity to squash commits to 1 😉

[astrale-sharp]

Hi!

I added functionality to filter test, i still need to update the chech.sh docs but you can now run ./check.sh itest substring, (it would only run test containing substring)
if you want it in another PR just say so, no problem.

added basic skip attributes using default, next part is support for specifiying the default in your attribute.

[Bromeon]

I added functionality to filter test, i still need to update the chech.sh docs but you can now run ./check.sh itest substring, (it would only run test containing substring)
if you want it in another PR just say so, no problem.

Yes, please don't mix unrelated functionality in the same PR unless it was previously agreed. This PR should ideally focus only on the skip changes, as advertised in the title.

but you can now run ./check.sh itest substring

This syntax is used for listing the tasks that check.sh has to run: test, itest, clippy, doc etc. So it would be confusing to recycle it for filtering. An explicit --filter argument would be more intuitive (but let's discuss this in another PR).

---

In general, it would be nice if I didn't have to repeat comments I already gave multiple times before. One of them is the formatting of quote! { ... } macros, which I mentioned at least four times in your last PR (1 2 3 4). Unfortunately, this PR suffers from the exact same issues.

Reviewing pull requests takes a lot of time during which I cannot work on implementation, so I would appreciate if I could spend this time on more meaningful issues. We try to automate whatever we can for bikeshedding topics like formatting, but rustfmt isn't perfect, and so I rely on user cooperation. I know this wasn't on purpose, but please write those review comments down, use the "mark as resolved" feature or any other system that works for you. Thanks a lot for your understanding!

[Bromeon]

Regarding the failing notify-docs workflow, it uses the runner's default Rust version for speed.

Keep in mind though that our MSRV is currently 1.66.0, so the full CI would fail with newer features. Theoretically we can update it, but I also don't want to always chase the latest release and force everyone to constantly update Rust, if it's only about smaller convenience features. Maybe the faster crates.io index warrants an update to 1.70 though 🤔

bors try

[bors]

try

Build failed:

• full-ci

[astrale-sharp]

My preferred solution is if you unsubscribe from my draft and then I @ you when I'm ready!

[astrale-sharp]

after overriding my version to 1.66.0, i get clippy errors from changes not related to mine, should I fix them? (2 very easy fix)

EDIT : problem doesn't appear on github checks cause the rust version for clippy if 1.70.0

error: use of `unwrap_or` followed by a function call
   --> itest/rust/src/virtual_methods_test.rs:538:14
    |
538 |             .unwrap_or(Variant::nil())
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(Variant::nil)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call
    = note: `-D clippy::or-fun-call` implied by `-D clippy::perf`

error: use of `unwrap_or` followed by a call to `new`
   --> itest/rust/src/runner.rs:248:10
    |
248 |         .unwrap_or(Array::new())
    |          ^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_default()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call

[astrale-sharp]

In the future I'd like to add support for a
#[variant( skip (default = user_func()))]
Should I add it in this pr ?

if so we'd need to discuss the syntax, a few come to mind ( I'll copy this message elsewhere if you'd rather have a separate pr)

struct UserStruct {
        #[variant( skip = (default = %value% ) )]
        field : ty
        #[to_variant( skip = (default = %value% ) )] // same but only skip 
}

I feel like there is a better way to do this

[Bromeon]

In the future I'd like to add support for a
#[variant( skip (default = user_func()))]
Should I add it in this pr ?

No, let's discuss separately. This PR is already open for 1.5 months, I'd like to wrap it up asap...

[astrale-sharp]

Then I believe there's only roundtrip_with_skip to decide and then the pr should be good !

[Bromeon]

Unfortunately there are still tons of formatting issues. Please, please go through all your code and make sure macros are consistently formatted. In case of doubt, look at existing code. I'm sorry to be pedantic here, but we have to maintain the quality of code for readers and the alternative is that I have to fix all those little things myself. I'm also really disappointed in rustfmt to not catch any of these; might consider using an alternative formatter in the future...

Don't use match if you only have true/false branches; this is what if is for.

The last thing: it would be nice if this macro got some documentation, otherwise you spent all that effort any people may not even discover this API. https://godot-rust.github.io/docs/gdext/pr-251/godot/bind/derive.ToVariant.html is currently empty. No need to write a novel, but maybe quickly elaborate the purpose and include 3-4 examples as doctests (you can even take those from the tests). Same for FromVariant. Also explain the #[skip] in particular.

[astrale-sharp]

caught this with regexes

[astrale-sharp]

I used a set of regexes to help me, hoping i caught all the formatting problems.

I don't have much motivation to keep working on this.

[Bromeon]

I don't have much motivation to keep working on this.

Fully understandable. Maybe some tips for the future:

• Split the PR into smaller parts. This turned out to be quite an involved feature, but you could for example have started only with #[variant(skip)] for structs. This would reduce the amount of code to review, the amount of changes to address, and the overall work involved. Don't think you are expected to do "the full thing", a lot of additions are helpful in a partial state and can be extended later, by you or someone else.
• Motivation is generally higher if the time between iterations is shorter. After several months, a lot of previous points are forgotten, it needs more time to re-think into the problem, etc. I understand there may be all sorts of RL reasons that delay pull requests, but having a smaller PR (point 1) would have the biggest impact here.
• Regarding the formatting stuff, I don't like to nitpick a lot, but at the same time please consider that work that is not done during the PR likely has to be done at some point in the future, and likely by me 😬 so I'd like to keep a level of code quality, if only because it helps other contributors navigate and recognize a consistent style. I hope that makes sense.

Thank you very much for your hard work on this feature addition, as well as the patience and persistence! 👍

bors r+

[bors]

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

• full-ci