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

Make Dynamic::try_cast_raw public #866

Merged
merged 1 commit into from
May 3, 2024
Merged

Conversation

paholg
Copy link
Contributor

@paholg paholg commented Apr 30, 2024

This function is useful for error-reporting. With try_cast, one has to do one of the following:

  1. Pre-clone the Dynamic.
  2. Check Dynamic::is first, and still handle the None case of try_cast.

@schungx
Copy link
Collaborator

schungx commented Apr 30, 2024

You have a better name for try_cast_raw? I tend to try to keep _raw for internal API or some really advanced API...

@paholg
Copy link
Contributor Author

paholg commented Apr 30, 2024

Fair enough. If I could go back in time, I would propose this function be try_cast and the one that returns an option be renamed or removed, but that is not worth a breaking change.

I can't think of a great name. Maybe try_cast_res or try_cast_result?

@schungx
Copy link
Collaborator

schungx commented May 1, 2024

Picking a good name is important for an API, just for that reason you just mentioned -- we can never ever change it.

Let's spend a couple more days thinking up new names... there is no rush until the next release anyway...

I also think try_cast_result looks reasonable...

This function is useful for error-reporting. With `try_cast`, one has to
do one of the following:
1. Pre-clone the `Dynamic`.
2. Check `Dynamic::is` first, and still handle the `None` case of
   `try_cast`.

To maintain the pattern of `_raw` for internal functions, we rename it
to `try_cast_result`.
@paholg
Copy link
Contributor Author

paholg commented May 2, 2024

I have updated the name to try_cast_result.

@schungx
Copy link
Collaborator

schungx commented May 3, 2024

Alright let's merge this.

@schungx schungx merged commit 390d55a into rhaiscript:main May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants