Stream: git-cranelift

Topic: cranelift / PR #1335 Allow 'clif-util run' to read stdin ...

GitHub (Jan 10 2020 at 22:31):

zummenix opened PR #1335 from clif-util-stdin to master:

This enables clif-util run - to work with stdin (seems it was intended initially).

Now errors in iterate_files are propagated further.

Closes #1004

GitHub (Jan 10 2020 at 22:54):

abrown submitted PR Review.

GitHub (Jan 10 2020 at 22:54):

abrown created PR Review Comment:

I was a bit confused reading this but tell me if I understand this correctly: if we pass "-" inside files then WalkDir will give us an error; however, we _can_ turn "-" into a valid PathBuf by doing e.path().into()?

GitHub (Jan 11 2020 at 05:03):

zummenix submitted PR Review.

GitHub (Jan 11 2020 at 05:03):

zummenix created PR Review Comment:

Yes, that's correct. We propagate all paths that WalkDir wasn't able to deal with to read_single_file function. I agree this is a bit clever and confusing but I wasn't able to find a better way.

GitHub (Jan 13 2020 at 19:10):

abrown submitted PR Review.

GitHub (Jan 13 2020 at 19:10):

abrown created PR Review Comment:

What about the following approach: why don't we separate out any strings that we know to have special semantics and chain them on the end of the iterated files? E.g.:

let (normal_files, special_files) = drain_specials(files);
for file in iterate_files(normal_files).chain(special_files)

Where drain_specials could be something like (there's surely a better way):

/// This could use `drain_filter` instead but it is a nightly-only experimental API.
fn drain_specials(mut files: Vec<String>) -> (Vec<String>, Vec<String>) {
    let mut normals = vec![];
    let mut specials = vec![];
    for s in files.drain(..) {
        match s.as_str() {
            "-" => specials.push(s),
            _ => normals.push(s),
        }
    }
    (normals, specials)
}

The reason I propose something like this is to avoid the catch-and-then-use of e.path().into() which feels hard to debug (and to read too, as you mention).

GitHub (Jan 13 2020 at 19:11):

abrown edited PR Review Comment.

GitHub (Jan 15 2020 at 16:10):

zummenix created PR Review Comment:

Does it make sense to have multiple stdin inputs? For example with clif-util run - - user fills in first input and then second. If not, then we can check if stdin arg exist, filter out all the - from files and chain the - at the end as you have proposed.

GitHub (Jan 15 2020 at 16:10):

zummenix submitted PR Review.

GitHub (Jan 15 2020 at 16:46):

abrown submitted PR Review.

GitHub (Jan 15 2020 at 16:46):

abrown created PR Review Comment:

Yeah, to me deduplicating special things like - makes sense.

GitHub (Jan 17 2020 at 04:38):

zummenix updated PR #1335 from clif-util-stdin to master:

This enables clif-util run - to work with stdin (seems it was intended initially).

Now errors in iterate_files are propagated further.

Closes #1004

GitHub (Jan 17 2020 at 17:36):

abrown submitted PR Review.

GitHub (Jan 17 2020 at 17:40):

abrown merged PR #1335.

Last updated: Nov 22 2024 at 17:03 UTC