Change free field to Vec<OsString>

[197g]

Sketched with the interface as proposed here: #97 (comment) except that I made free() a Result<Vec<_>, Fail>. Since the argument processing would usually return an error before in the constructor and we no longer have that verification, a panic might be surprising to callers that had previously relied on that.

In my opinion the necessary doctest changes suggest this is not super ergonomic yet. I've not yet added a lot of documentation to the methods in case the interface still needs changes.

Additionally it seems odd to treat Val and free arguments differently but that might be a tradeoff worth making. Your call.

[tgross35]

Thanks for looking at this!

In my opinion the necessary doctest changes suggest this is not super ergonomic yet. I've not yet added a lot of documentation to the methods in case the interface still needs changes.

The doctests can use .free() rather than .free_os() right? That's what we will probably expect most users to do, free_os only exists in case you actually expect to be handling non-UTF-8 strings.

[tgross35]

As part of that, I think .free() should panic and suggest using free_os() rather than returning a Result.

(I'm not 100% convinced of this yet, it's just more ergonomic)

[197g]

It's not really the OsString which is awkward to handle. That part seemed fine and by wrapping the expected side in an OsStr::new constructor the comparison does not stand out much. What bothers me, at least now I'm able to put it into words better, is the way of accessing individual arguments if that were written with free:

    /// assert_eq!(matches.free()[end_pos], "arg2");
                           ^^^^^^^^^^^^^^^ indexing into temporary Vec

//!         matches.free()[0].clone()
                    ^^^^^^^^^^^^^^^^^ here too

It feels really dirty to do the full allocation when the documentation demonstrates mostly individual argument access. Maybe it should utilized an iterator instead (but when to panic then)?