Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generated code depends on functions only available under feature convenience #60

Closed
chitoyuu opened this issue Jan 8, 2023 · 2 comments · Fixed by #113
Closed

Generated code depends on functions only available under feature convenience #60

chitoyuu opened this issue Jan 8, 2023 · 2 comments · Fixed by #113
Labels
bug c: core Core components

Comments

@chitoyuu
Copy link

chitoyuu commented Jan 8, 2023

Screenshot from hapenia on Discord:

@chitoyuu chitoyuu changed the title Generated code depends on Variant::to which is only available under feature convenience Generated code depends on functions only available under feature convenience Jan 8, 2023
@Bromeon Bromeon added bug c: core Core components labels Jan 15, 2023
@Bromeon
Copy link
Member

Bromeon commented Jan 15, 2023

Thanks! I think if we want to keep using that feature, we should probably run tests with and without it in CI.

@Bromeon
Copy link
Member

Bromeon commented Jan 16, 2023

Originally the intention of convenience (possibly to be renamed as yolo 😀) was that someone who wants to be aware of fallible operations can disable that feature and the number of panicking functions will be drastically reduced.

That said, I'm not sure if there are people truly wishing for that or if it's something we want to encourage as a best practice, as it has two notable downsides:

  • Verbosity of code that never fails due to external invariants.
    • E.g. scene tree has a certain structure, so getting a node of a given name/type must succeed or it's a bug.
    • If I know the value on GDScript side fits into a u8, then to_variant() must succeed or it's a bug.
  • Worse error messages.
    • People tend to use unwrap() or expect("literal"), which lacks context information.
    • get_node_as::<T>("path") on the other hand can include a precise panic message, e.g. "there is a node at path path with type U, but type T was requested".

Maybe I'll remove this feature but keep the commented-out #[cfg] statements around in case we want to re-introduce it later. Keeping it may add quite some maintenance overhead which is probably not justifiable at this stage of the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: core Core components
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants