Addition of step in export_range

[StatisMike]

Duplicate of #483

Resolves #413

[Bromeon]

For the future, you can just (force-)push to the same branch again. As long as there is at least one commit on that branch, you should be able to reopen the PR 🙂

[GodotRust]

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

[StatisMike]

@Bromeon It seems I've botched something big time on my fork and lost all work. Remade it from memory, as most of the time was spend of figuring out how all this interacts with each other.

As it was such a big mess, it seemed optimal safest to me to make it a new PR (last one was closed automatically - as in I didn't ever clicked the Close button - because of the error I've done on my fork).

[Bromeon]

With git, it's rather hard to permanently lose work, as soon as things are committed. You could look into git reflog to recover past commits, even if you've lost the branch itself. This might not be relevant now anymore, but could help you another time 🙂

Furthermore, GitHub also retains commits, see the timeline in the old PR.
Yours was probably this one? f30510b

[StatisMike]

Yeah, it seems like I urgently need to step up my git flow knowledge :P

[Bromeon]

Thanks!

The is_next_ident is very specific to only this one use case. If somehow possible, I'd like to avoid the proc-macro parsers to become huge patchworks of special cases; it's bad enough that we had to duplicate KvParser and ListParser.

We already have try_next_ident, can you not use that to check whether the next expression is an ident?

[StatisMike]

Using try_next_ident here makes the current implementation with the following option parsing break -> if there IS an identifier, the parser would pop the next token. To make minimal changes in the present implementation, I needed a method that would never consume the next token, just take a peek and check the next token - that was the rationale behind is_next_ident

I am not sure that this is the only place where it will be ever needed - didn't look through all other export annotations to check if there are optional literal arguments in there that could be omitted in the past. Moreover - if there is not one now, Godot development is still ongoing and who knows if there wouldn't be one in the future. It seemed more reasonable to do it that way than refactoring the following retrieval of the options identifiers

[Bromeon]

Using try_next_ident here makes the current implementation with the following option parsing break -> if there IS an identifier, the parser would pop the next token. To make minimal changes in the present implementation, I needed a method that would never consume the next token, just take a peek and check the next token - that was the rationale behind is_next_ident

I see. Maybe it should then be named peek_next_ident() and directly return the Ident, instead of requiring a 2nd query.

---

Moreover - if there is not one now, Godot development is still ongoing and who knows if there wouldn't be one in the future.

This argument can be used to justify any possible feature. There's no concrete indication that Godot plans to add more range parameters, so I'd rather follow YAGNI. (This as a general note, not regarding the feature here in particular).

---

It seemed more reasonable to do it that way than refactoring the following retrieval of the options identifiers

Yeah, I think it's fine for now 🙂 I will try to refactor these parsers a bit in the future, maybe we can also share some logic between KV and list parsers.

[Bromeon]

To have all change requests as suggestions:

• Could be named peek_next_ident and return the Ident directly.
• Full stops and line lengths in docs 😉

[StatisMike]

I don't really think that peek_next_ident will actually work.

The thing is in the #[export_range(min, max, step, options..)] the min and max are the only required elements. Next comes step, which (I think?) need to be retrieved using parser.next_expr()?. If there were one of the options (which are idents) it will be retrieved as expression - so parser.next_expr()? won't throw error, the error would be from general compiler if symbol of the same name as option identifier won't be present in current scope.

I wanted to introduce parser.try_next_expr() there, but it wouldn't work because of the above. Instead I needed to do reverse check - if there is identifier, we can omit getting the step, and we can move to option parsing.

Later, at the other hand the options are parsed using loop with parser.next_any_ident(&ALLOWED_OPTIONS[..])? which is very clear and clean, but unfortunately needs the parser to have all the option identifiers not consumed. If I would retrieve the identifier beforehand, it wouldn't be there.

MID-WRITING IDEA
So, during writing this comment I begin to think in some other way, and came up with an alternative skipping the need for is_next_ident :

        let step = match parser.next_any_ident(&ALLOWED_OPTIONS[..]) {
            Ok(maybe_option) => {
                if let Some(option) = maybe_option {
                    options.insert(option.to_string());
                }
                quote! { None }
            },
            Err(_) => {
                let value = parser.next_expr()?;
                quote!{ Some(#value) }
            }
        };

But I'm not completely sure that it will work in all scenarios, seems to pass the tests though...

Edit: Yeah, it makes in the scenario if the first option given after #[export_range(min, max, my_option)] isn't one of ALLOWED_OPTIONS the error message wouldn't be clear.

[Bromeon]

So what do you suggest?

As mentioned, let's just get this working for now, we can refactor the parsers later... Important is that the cases are covered by tests so that any future changes don't break things.

[StatisMike]

Currently the only idea I have is the explicit check for the identifier, as in initial commit with is_next_ident, without consuming or retrieving it in any way. It has some overhead and adds next method, needed exclusively for this functionality, but keeps the parsing with explicit and clean error messages.

Alternatively could just expose peek() method in ListParser and move the whole is_next_ident method implementation into new_range_list(). It should be safe, I think, as peek() doesn't mutate the ListParser.

[Bromeon]

Currently the only idea I have is the explicit check for the identifier, as in initial commit with is_next_ident, without consuming or retrieving it in any way. It has some overhead and adds next method, needed exclusively for this functionality, but keeps the parsing with explicit and clean error messages.

That's fine for me 🙂 I'm not too worried about the small overhead at this point, but eventually would like to consolidate the implementation a bit (not in this PR).

So if once you address the other suggestions in the code, it should be ready to merge from my point 👍

[StatisMike]

@Bromeon I think I've adressed all other issues mentioned, it seems to be ready to merge?

[Bromeon]

Almost good, please squash the commits to one. And let me know if you need help with it!

[StatisMike]

@Bromeon I think I've done it correctly this time! I found this strategy: https://stackoverflow.com/a/69827502 to be much clearer to me than venturing into a rebase hellish circle :)

[Bromeon]

Thanks a lot!