r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

Looks good to me, thanks! Could you add a test for this as well?

Sure, will do!

Also, it would make sense to allow for the same syntax in the target.<...> section, I think I'll add that as well.

While building the test case, I noticed that some moving part outside of cargo is doing config file validation, killing the process with an error message before my code runs:

error: invalid configuration for key `build.rustflags`
expected a list, but found a string for `build.rustflags` in /path/here

I'll look into it now.

EDIT: It is indeed a part in cargo, but the message shouldn't be printed, given we want to retry with a different lookup.

Oh I think it's the get_list(key)?, you'll want to change that to handle the error by looking up a string instead

Yes, I figured as much :). Still needs a way to generate a better error message signalling the user can use both strings and lists.

It will probably be somewhat longer and less elegant, however.

Hm, after fiddling around with the code a bit, I think this is the cleanest implementation:

    match config.values()?.get(&key) {
        Some(&ConfigValue::String(ref arg, _)) => {
            return Ok(arg.split(' ').map(str::to_string).collect());
        },
        Some(&ConfigValue::List(ref args, _)) => {
            let args = args.into_iter().map(|a| a.0.clone());
            return Ok(args.collect());
        },
        Some(value) => return config.expected("list or string", &key, value.clone()),
        None => (),
    }

Ah I prefer to use get_string and get_list wherever possible as it works with the env vars (where the values() method doesn't right now

Yes, that's indeed a pitfall. I guess it won't work without a third lookup for the value, however, if we want a custom error message (which I think would make sense). Or I could implement something like get_string_or_list with all the functionality as in get_string and get_list and use that, but I'm not sure whether that's worth it, given that there would be only these two calls to it. It might prove handy in the future, however.

Oh runtime we don't really have to worry about here, just errors and such.

Looks good to me, thanks! Want to go ahead and refactor this to perhaps get_list_or_maybe_string or something like that?

Sure. I also still need to figure out the details around the second test failing.

This should, when passing all tests, be a complete solution in my book ^^.

@bors: r+

Thanks!

📌 Commit f440704 has been approved by alexcrichton

⌛ Testing commit f440704 with merge 4aef11e...

💔 Test failed - status-appveyor

⌛ Testing commit f440704 with merge c2852b1...

💔 Test failed - status-travis

@bors: retry

⌛ Testing commit f440704 with merge f33b7b0...

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing f33b7b0 to master...

Thanks for having me!