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

Export Optionality markers #203

Merged
merged 2 commits into from
Jun 29, 2024

Conversation

zmbush
Copy link
Contributor

@zmbush zmbush commented Jun 29, 2024

In order to support custom implementations of YarnFnParam, Optional and Required need to be exported.

This exports yarnspinner_core::yarn_fn::optionality as yarnspinner_core::prelude::optionality and seals the AllowedOptionalityChain trait so it cannot be implemented.

In order to support custom implementations of `YarnFnParam`, `Optional`
and `Required` need to be exported.

This exports `yarnspinner_core::yarn_fn::optionality` as
`yarnspinner_core::prelude::optionality` and seals the
`AllowedOptionalityChain` trait so it cannot be implemented.
@janhohenheim
Copy link
Member

Huh, I was even considering explicitly sealing YarnFnParam. I'm curious, why do you need a custom YarnFnParam implementation?

@zmbush zmbush force-pushed the expose-optionality branch from 18c9ea9 to a0ae393 Compare June 29, 2024 19:23
@bash
Copy link
Collaborator

bash commented Jun 29, 2024

@janhohenheim If you decide to merge this then I intend to do a follow-up PR to change Optionality to be an enum / associated const. I think that would make for a nicer public API than the current approach :)

@zmbush Would you be okay with me pushing that change onto this PR?

@zmbush
Copy link
Contributor Author

zmbush commented Jun 29, 2024

It's nice to have for convenience. I have a bunch of types that I use as command arguments, and instead of taking them as Strings, it's nice to be able to do something like:

    #[yarn_command(dialog_runner)]
    fn set_background(
        In((new_bg, mode)): In<(Location, ShowOption)>,
        ...
    ) { 
        ...
    }

instead of

    #[yarn_command(dialog_runner)]
    fn set_background(
        In((new_bg, mode)): In<(String, String)>,
        ...
    ) { 
        let new_bg: Location = new_bg.parse().unwrap();
        let mode: ShowOption = mode.parse().unwrap();
        ...
    }

@zmbush
Copy link
Contributor Author

zmbush commented Jun 29, 2024

@zmbush Would you be okay with me pushing that change onto this PR?

@bash Yeah, go ahead!

@janhohenheim
Copy link
Member

@zmbush I see! I was thinking about making automatic parsing a feature as well, so this workaround is completely valid. I'll merge @bash's counteroffer when it's ready :)

Btw, what's that #[yarn_command(dialog_runner)]? It looks nifty. I assume it expands to something like this?

fn register_set_background(mut dialog_runner: Query<&mut DialogueRunner, Added<DialogueRunner>>) {
  for mut dialog_runner in dialog_runner.iter_mut() {
    dialog_runner.commands_mut().add_command("set_background", set_background);
  }
}

@zmbush
Copy link
Contributor Author

zmbush commented Jun 29, 2024

@janhohenheim Yeah, that's essentially what it does. It actually started as a way to generate ysls.json files for commands so that the language server would be able to detect them. I can try to upstream it if you're interested. (This is how the current version is used: https://gist.github.com/zmbush/f23752ac3453e9ff7d3173ca8e11f898#file-yarn_command_demo-rs)

@janhohenheim
Copy link
Member

janhohenheim commented Jun 29, 2024

Yeah, generating ysls.json is also on my to do list 😅 I'll look into it once Yarn Spinner V3 launches at the latest because I'll need to port a whole bunch of stuff by then anyways. It should be possible to generate it without macros because we have runtime info on both the function name and its signature (well, other than the parameter names, but that's not important for the language server).

The macro looks nice! I wouldn't upstream it since I prefer to expose exactly one way to configure stuff, but I am tempted to replicate it for my own project :D

BTW, please ping me if you publish anything using the crate, I'd love to see what cool stuff people create with it :)

@bash
Copy link
Collaborator

bash commented Jun 29, 2024

change Optionality to be an enum / associated const

Never mind, associated consts and where clauses do not like each other :/

@janhohenheim
Copy link
Member

@zmbush I assume sealing Optionality does not break your workflow?

@zmbush
Copy link
Contributor Author

zmbush commented Jun 29, 2024

@janhohenheim / @bash Sealing Optionality still works for my uses.

@janhohenheim janhohenheim merged commit da56041 into YarnSpinnerTool:main Jun 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants