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

Replace PathBuf to AsRef<Path> in the Engine file API #693

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

starovoid
Copy link

I also tested the changes with the following code:

#[cfg(test)]
mod test {
    use super::*;
    use std::path::PathBuf;

    #[test]
    fn test_replace_path_buf() {
        let engine = Engine::new();
        let mut scope = Scope::new();

        let ast = engine.compile_file(Path::new("script.rhai"));
        let ast = engine.compile_file(PathBuf::from("script.rhai"));
        let ast = engine.compile_file("script.rhai");
        let ast = engine.compile_file("script.rhai");

        let ast = engine.compile_file_with_scope(&mut scope, Path::new("script.rhai"));
        let ast = engine.compile_file_with_scope(&mut scope, PathBuf::from("script.rhai"));
        let ast = engine.compile_file_with_scope(&mut scope, String::from("script.rhai"));
        let ast = engine.compile_file_with_scope(&mut scope, "script.rhai");

        let res = engine.eval_file::<i64>(Path::new("script.rhai"));
        let res = engine.eval_file::<i64>(PathBuf::from("script.rhai"));
        let res = engine.eval_file::<i64>(String::from("script.rhai"));
        let res = engine.eval_file::<i64>("script.rhai");

        let res = engine.eval_file_with_scope::<i64>(&mut scope, Path::new("script.rhai"));
        let res = engine.eval_file_with_scope::<i64>(&mut scope, PathBuf::from("script.rhai"));
        let res = engine.eval_file_with_scope::<i64>(&mut scope, String::from("script.rhai"));
        let res = engine.eval_file_with_scope::<i64>(&mut scope, "script.rhai");

        let res = engine.run_file(Path::new("script.rhai"));
        let res = engine.run_file(PathBuf::from("script.rhai"));
        let res = engine.run_file(String::from("script.rhai"));
        let res = engine.run_file("script.rhai");

        let res = engine.run_file_with_scope(&mut scope, Path::new("script.rhai"));
        let res = engine.run_file_with_scope(&mut scope, PathBuf::from("script.rhai"));
        let res = engine.run_file_with_scope(&mut scope, String::from("script.rhai"));
        let res = engine.run_file_with_scope(&mut scope, "script.rhai");
    }
}

The use of PathBuf has been replaced with AsRef<Path> in the file API of the engine. An implementation of `From<&Path>` for Immutable `String` has been added to satisfy type constraints in `ast` module.
@schungx
Copy link
Collaborator

schungx commented Jan 22, 2023

I suppose it would break existing code that uses syntax like "script.rhai".into() ?

@starovoid
Copy link
Author

Yes, because now the compiler does not know which type we want to pass. But this can be easily fixed by removing ".into()".

@schungx
Copy link
Collaborator

schungx commented Jan 24, 2023

Yes, because now the compiler does not know which type we want to pass. But this can be easily fixed by removing ".into()".

True, but that would break a lot of existing code since I recommended .into() in the examples.

I have been trying to find a way to do it generic while maintaining compatibility with .into()...

@schungx schungx linked an issue Jan 24, 2023 that may be closed by this pull request
@starovoid
Copy link
Author

starovoid commented Jan 24, 2023

Yes, because now the compiler does not know which type we want to pass. But this can be easily fixed by removing ".into()".

True, but that would break a lot of existing code since I recommended .into() in the examples.

I have been trying to find a way to do it generic while maintaining compatibility with .into()...

I think we can try to add a new type that implements both AsRef and Into traits for various types being passed. But isn't there any less complex way?

@schungx
Copy link
Collaborator

schungx commented Jan 30, 2023

I think we can try to add a new type that implements both AsRef and Into traits for various types being passed. But isn't there any less complex way?

Yeah, I have been wondering about that also.

So unfortunately this will still be a major API-breaking change unless we can find ways to get rid of the Into error.

@starovoid
Copy link
Author

I think we can try to add a new type that implements both AsRef and Into traits for various types being passed. But isn't there any less complex way?

Yeah, I have been wondering about that also.

So unfortunately this will still be a major API-breaking change unless we can find ways to get rid of the Into error.

When is the release of the new major version planned?

@schungx
Copy link
Collaborator

schungx commented Feb 2, 2023

When is the release of the new major version planned?

None has been planned so far... unless there is a good reason to introduce major API breaks which should mean some significant changes.

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.

Why does Engine::eval_file_with_scope use PathBuf instead of Path?
2 participants