Skip to content
This repository has been archived by the owner on Sep 24, 2022. It is now read-only.

Add lookup_mut method for mutable access #88

Merged
merged 3 commits into from Mar 25, 2016

Conversation

kstrafe
Copy link
Contributor

@kstrafe kstrafe commented Mar 24, 2016

Mutable access may sometimes be desired in order to change values
in the toml table. This can be used for dynamic configurations which
will be easy to modify and store.

lookup_mut requires a recursive method due to the borrow checker
not allowing to have more than one mutable reference in the same
scope.

Mutable access may sometimes be desired in order to change values
in the toml table. This can be used for dynamic configurations which
will be easy to modify and store.

lookup_mut requires a recursive method due to the borrow checker
not allowing to have more than one mutable reference in the same
scope.
@alexcrichton
Copy link
Collaborator

Thanks! Could this have the same non-recursive implementation as lookup?

@kstrafe
Copy link
Contributor Author

kstrafe commented Mar 24, 2016

I've tried but the borrow checker seems to complain. Asked in the irc and someone suggested a recursive solution. I think it's because there are two mutable references generated whilst the cur_value gets overwritten. Maybe the borrow checker chokes on this.

@alexcrichton
Copy link
Collaborator

Ah yeah sometimes you have to do something like:

let tmp = value;
value = &mut tmp.foo

(or something like that)

Do you have a non-recursive version of the code I could peek at?

@kstrafe
Copy link
Contributor Author

kstrafe commented Mar 24, 2016

fn e_lookup_mut<'a>(value: &'a mut Value, path: &'a str) -> Option<&'a mut Value> {
    let mut cur_value = value;
    if path.len() == 0 {
        return Some(cur_value)
    }

    for key in path.split('.') {
        match *cur_value {
            Value::Table(ref mut hm) => {
                match hm.get_mut(key) {
                    Some(v) => cur_value = v,
                    None => return None
                }
            },
            Value::Array(ref mut v) => {
                match key.parse::<usize>().ok() {
                    Some(idx) if idx < v.len() => cur_value = &mut v[idx],
                    _ => return None
                }
            },
            _ => return None
        }
    };

    Some(cur_value)
}

This yields the same error as before. Unfortunately I didn't commit any attempts at the non-recursive solution.

@alexcrichton
Copy link
Collaborator

I think this should work:

pub fn lookup_mut(&mut self, path: &str) -> Option<&mut Value> {     
    let mut cur = self;                                              
    if path.len() == 0 {                                             
        return Some(cur)                                             
    }                                                                

    for key in path.split('.') {                                     
        let tmp = cur;                                               
        match *tmp {                                                 
            Value::Table(ref mut hm) => {                            
                match hm.get_mut(key) {                              
                    Some(v) => cur = v,                              
                    None => return None                              
                }                                                    
            }                                                        
            Value::Array(ref mut v) => {                             
                match key.parse::<usize>().ok() {                    
                    Some(idx) if idx < v.len() => cur = &mut v[idx], 
                    _ => return None                                 
                }                                                    
            }                                                        
            _ => return None                                         
        }                                                            
    }                                                                

    Some(cur)                                                        
}                                                                    

@kstrafe
Copy link
Contributor Author

kstrafe commented Mar 25, 2016

Seems like it does!
I guess this can be closed.

@alexcrichton
Copy link
Collaborator

Oh I was actually thinking you may want to use that for this PR :)

Could you add some various tests here and there for the method as well?

Also include some tests simply by copying and modifying
the other tests for lookup.
@kstrafe
Copy link
Contributor Author

kstrafe commented Mar 25, 2016

I think this should do it :)
Are there any plans for implementing lookup with table names as "table.name" (where " delimits the name)? As in [table."my.name"]

@alexcrichton
Copy link
Collaborator

Thanks! Yeah I'd be fine using the same TOML syntax for key lookups, just haven't implemented it yet :)

@alexcrichton alexcrichton merged commit b0a134a into toml-rs:master Mar 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants