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

Graham/fix nix output parsing #137

Merged
merged 6 commits into from
Aug 16, 2019

Conversation

grahamc
Copy link
Contributor

@grahamc grahamc commented Aug 15, 2019

The code in #133 is pretty good, check out these proposed patches for some feedback / suggestions

This borrows upstream Rust code to implement a Lines iterator which produces OsStrings, and replaces the parse_nix_output code to use it. From the commit where I drop that function:

nix: drop parse_nix_output

Prefer the iterator format, especially when we're able to
use familiar forms of passing functions around.

In this case the parse_nix_output function probably shouldn't
skip newlines.

Additionally, reducing the powerful model of iterators down
to only supporting (and actually requiring) a single function
isn't doing us much favor.

Finally, in the tests for the builder I add a proof-positive test to assert that the bytes come through and that the shell derivation is produced.

Prefer the iterator format, especially when we're able to
use familiar forms of passing functions around.

In this case the parse_nix_output function probably shouldn't
skip newlines.

Additionally, reducing the powerful model of iterators down
to only supporting (and actually requiring) a single function
isn't doing us much favor.
@grahamc grahamc requested a review from Profpatsch August 15, 2019 20:14
src/builder.rs Show resolved Hide resolved
src/builder.rs Show resolved Hide resolved
src/builder.rs Show resolved Hide resolved
src/nix.rs Show resolved Hide resolved
src/osstrlines.rs Show resolved Hide resolved
@Profpatsch Profpatsch merged commit 92d3587 into fix-nix-output-parsing Aug 16, 2019
@Profpatsch Profpatsch deleted the graham/fix-nix-output-parsing branch August 16, 2019 16:24
@Profpatsch Profpatsch mentioned this pull request Nov 18, 2019
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.

3 participants