Codecov Report

Merging #75 into master will increase coverage by 0.11%.
The diff coverage is 85.21%.

@@            Coverage Diff             @@
##           master      #75      +/-   ##
==========================================
+ Coverage   80.46%   80.58%   +0.11%     
==========================================
  Files          18       18              
  Lines        2580     2678      +98     
==========================================
+ Hits         2076     2158      +82     
- Misses        504      520      +16     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67e8500...5aa9255. Read the comment docs.

@hackerchai We don't need to change &str to String in parameters. https://hermanradtke.com/2015/05/03/string-vs-str-in-rust-functions.html

Only change the returned &str is enough.

Hello @xcaptain, here I found a good post which explains why String is better than &str in some situations.

rust-lang/rust#46966
projectfluent/fluent-rs#123

I think using &str in casbin-rs is not realistic, although it may be more efficient.

Image that a client generated a Vec as policies, and he wants to use in casbin-rs, he has to converted Vec to Vec<&str>. That's not convenient.

In realworld example, we always get Vec<String> from file system, network etc. We often just generate Vec<String> over Vec<&str>

An example of code is the following which I encountered in my personal project:

fn load_records<P: AsRef<Path>>(p: P) -> Vec<StringRecord> {
    if let Ok(mut rdr) = ReaderBuilder::new()
        .has_headers(false)
        .double_quote(false)
        .comment(Some(b'#'))
        .delimiter(b',')
        .trim(Trim::All)
        .from_path(p)
    {
        if let Ok(records) = rdr.records().collect::<Result<Vec<StringRecord>, Error>>() {
            return records
                .into_iter()
                .filter(|r| is_valid_policy(r) || is_valid_grouping_policy(r))
                .collect::<Vec<StringRecord>>();
        }
    }

    vec![]
}

In the above code I was trying to read a csv file for getting default policies. I generated Vec<StringRecord> but it's not convenient to convert Vec<StringRecord> to Vec<&str>.

Then if I tried to do so, I'll got lifetime issue because the Vec<&str> will share the samle lifetime with generated StringRecord.

Ok, I agree to use owned string in all the public APIs.

Thanks @xcaptain and glad to see you agree with me!

Found a RFC here https://github.com/rust-lang/rfcs/blob/master/text/0241-deref-conversions.md

After it get implemented we don't need to worry whether use String or &str anymore, the compiler can help us.

I'm pretty sure auto-deref is implemented I think what it allows is

fn use_vector<T>(vec: &[T]) {}
fn use_string(s: &str) {}

let v = vec![0, 1, 2];
let s = String::new();

vector(&v);
use_string(&s);

@DevinR528 You are right, my mistake, RFC 241 has been merged.

I was think to write a function that can take String and &str at the same time. The deref trait makes &String equals to &str but not very idea.

How about we use AsRef https://doc.rust-lang.org/std/convert/trait.AsRef.html here, then we don't need to dereference a string manually?

@xcaptain @DevinR528 I think Into<String> + Send + Sync can help.

It's a common pattern to use Into<String> in other library. In my opinion we don't need it because in the most cases we just simply generate Vec<String>.

It's hard to find a situation in which we generated Vec<&str>. If we do have then it may be hand written &str which avoided lifetime issue because they are 'static.

@GopherJ as you know (you also tried to get the Api traits to take an Into<String>) it is way more trouble than it would be worth. I'm pretty sure to get any of the traits to use Into<String> we would have to Box it since its a dyn Into<string> and not boxing we may run into the same problem as with

pub struct Enforcer<A: Adaptor> {
    adaptor: A

where it only accepts one instance.

@DevinR528 Yes I agree, I removed static dispatcher because it causes some constraints.

We'll not be able to change types in runtime. (once instance is generated)

Anyway I think we should stick to Vec<String> before finding better solution

I think we can merge this. Thanks @hackerchai for your contributions!