NOT TO MERGE comments from nostarch on ch12 for review #563

carols10cents · 2017-03-25T00:52:48Z

No description provided.

carols10cents · 2017-03-25T00:54:23Z

second-edition/nostarch/chapter12.md

+We're calling our version 'greprs'.
+
+<!-- Unless I'm misunderstanding something, it seems like we start calling it
+merely "greps" at the end of the project, maybe something to look out for -->


I checked (with grep! ha!) and we don't say greps anywhere. Based on her comment later, I think she means just grep, not greprs, and she's confused because we name the function just grep. Maybe we should call that function "search" or something.

Looks like the first bullet point here may not have made it into RFC 1574, but perhaps you could follow that here to help differentiate when you're talking about the function rather than the entire program?

All the lines like:

the grep function

Could be something like:

the grep() function

Or possibly just:

grep()

Where appropriate.

Or maybe that doesn't match the rest of the book!

It looks like that wouldn't match the rest of the book!

And I finished reading your post, the suggestion of changing the function name is much better!

seconded, changing the name is probably better

carols10cents · 2017-03-25T00:55:04Z

second-edition/nostarch/chapter12.md

+<!-- We might need a more descriptive title, something that captures the new
+elements we're introducing -- are we going to cover things like environment
+variables more in later chapters, or is this the only place we explain how to
+use them? -->


Keep reading... later she actually suggests cutting the env var and stderr sections, so come back to this comment once we decide what's going to be in or out of this chapter.

carols10cents · 2017-03-25T00:55:44Z

second-edition/nostarch/chapter12.md

+- Running tests (Chapter 11)
+
+We'll also briefly introduce closures, iterators, and trait objects, which
+Chapters XX, YY, and ZZ respectively will cover in detail.


I will fill these in when I make edits

carols10cents · 2017-03-25T00:56:50Z

second-edition/nostarch/chapter12.md

+<!--Below -- I'm not clear what we need the args function for, yet, can you set
+it out more concretely? Otherwise, will it make more sense in context of the
+code later? Is this function needed to allow our function to accept arguments,
+is that was "args" is for? -->


Maybe show an example invocation of the command with arguments that we're going for.... that's the only thing I can think of since we explain that grep takes 2 command line arguments for the string and the file to search in the intro...

carols10cents · 2017-03-25T00:58:10Z

second-edition/nostarch/chapter12.md

-understand much about how they work in order to use them. We only need to
-understand two things:
+arguments that were given to our program. We haven't discussed iterators yet,
+and we'll cover them fully in Chapter 16, but for our purposes now we only need


carols10cents · 2017-03-25T01:19:51Z

second-edition/nostarch/chapter12.md

-to implement `grep`:
+Great, our test fails, exactly as we expected. Let's get the test to pass!
+
+### Testing to Succeed


making the test pass

carols10cents · 2017-03-25T01:21:39Z

second-edition/nostarch/chapter12.md

+We use a `for` loop along with the `lines` method to get each line in turn.
+
+<!-- so what does `lines` do on its own, if we need to use it in a for loop to
+work? -->


will add that it returns an iterator, reference some of the places where we've used a for loop before here to go through each item in a list

carols10cents · 2017-03-25T01:23:30Z

second-edition/nostarch/chapter12.md

@@ -1217,22 +1345,38 @@ To tell your name the livelong day
 To an admiring bog!
 ```

+<!-- Maybe this second one should be a more specific example, a whole word
+match, like "nobody"? These results are not immediately clear -->


i'll break this out more, say "let's try a search for nobody that should return exactly one line, ok, let's try something that should match multiple lines, ok now something that matches substrings of words"

carols10cents · 2017-03-25T01:24:55Z

second-edition/nostarch/chapter12.md

 Excellent! We've built our own version of a classic tool, and learned a lot
 about how to structure applications. We've also learned a bit about file input
 and output, lifetimes, testing, and command line parsing.

+<!-- If we'er going into environment variable more thoroughly in a later
+chapter, I might suggest we cut this next bit --- I'm not sure it will be


idk how i feel about this. i dont want to talk about it later, this is just like... useful formulas

I'm going to add a little thing that says to move on to chapter 13 if you want, but that we're going to briefly demonstrate env vars and stderr before calling this "done".

carols10cents · 2017-03-25T01:26:44Z

second-edition/nostarch/chapter12.md


 The `std::env` module contains many more useful features for dealing with
 environment variables; check out its documentation to see what's available.

+<!-- And this section, too, might be too much. We might be wearing the reader
+out at this point -->
+
 ## Write to `stderr` Instead of `stdout`


given that RFC for adding errln! or whatever is coming, maybe cutting this would be for the best...

until it lands though, knowing how to do this is important, and it won't change the section that much.

steveklabnik · 2017-03-27T14:41:41Z

second-edition/nostarch/chapter12.md

+We're calling our version 'greprs'.
+
+<!-- Unless I'm misunderstanding something, it seems like we start calling it
+merely "greps" at the end of the project, maybe something to look out for -->


seconded, changing the name is probably better

steveklabnik · 2017-03-27T14:42:20Z

second-edition/nostarch/chapter12.md

+<!--Below -- I'm not clear what we need the args function for, yet, can you set
+it out more concretely? Otherwise, will it make more sense in context of the
+code later? Is this function needed to allow our function to accept arguments,
+is that was "args" is for? -->


steveklabnik · 2017-03-27T14:42:55Z

second-edition/nostarch/chapter12.md

-a lone `args`. Also, if we end up using more than one function in `std::env`,
-we only need a single `use`.
+First, we bring the `std::env` module into scope with a `use` statement so that
+we can use its `args` function. Notice we have two environments here: the


also, we've already talked about this already, right?

steveklabnik · 2017-03-27T14:43:58Z

second-edition/nostarch/chapter12.md

-so we put a reference to the first argument in the variable `search`. The
-second argument will be the filename, so we put a reference to the second
-argument in the variable `filename`. Let's try running this program again:
+Remember, the program's name takes up the first argument at `args[0]`, so we


this is something we just mentioned above now, so repeating it feels like too much, idk

steveklabnik · 2017-03-27T14:44:32Z

second-edition/nostarch/chapter12.md

+the chapter.-->
+
+<!-- This might be more distracting than useful at this point, ok to cut this
+bit? I added a smaller summary line above -->


yeah, idk. i feel...whatever

steveklabnik · 2017-03-27T14:50:06Z

second-edition/nostarch/chapter12.md

 <!-- Will add ghosting and wingdings in libreoffice /Carol -->

+<!-- what does returning a Result rather than a Config do? -->


we have a whole chapter on this :(

steveklabnik · 2017-03-27T14:50:35Z

second-edition/nostarch/chapter12.md

+the unit type, `()`, and we keep that as the value returned in the `Ok` case.
+
+<!-- is just the `Box` bit the trait object, or the whole `Box<Error>`
+syntax?-->


the whole thing

steveklabnik · 2017-03-27T14:53:19Z

second-edition/nostarch/chapter12.md

+of the environment variable and its value.
+
+<!-- why do we use a loop rather than collect? what benefit does it have, and
+why use it here but not previously?-->


because collect creates a new collection, we want to just see if one thing exists

steveklabnik · 2017-03-27T14:54:26Z

second-edition/nostarch/chapter12.md


 The `std::env` module contains many more useful features for dealing with
 environment variables; check out its documentation to see what's available.

+<!-- And this section, too, might be too much. We might be wearing the reader
+out at this point -->


people want more and more stuff here, idk

steveklabnik · 2017-03-27T14:54:49Z

second-edition/nostarch/chapter12.md


 The `std::env` module contains many more useful features for dealing with
 environment variables; check out its documentation to see what's available.

+<!-- And this section, too, might be too much. We might be wearing the reader
+out at this point -->
+
 ## Write to `stderr` Instead of `stdout`


until it lands though, knowing how to do this is important, and it won't change the section that much.

carols10cents · 2017-03-28T16:31:43Z

Replaced by #576

NOT TO MERGE comments from nostarch on ch12 for review

2d898ae

carols10cents commented Mar 25, 2017

View reviewed changes

steveklabnik reviewed Mar 27, 2017

View reviewed changes

carols10cents mentioned this pull request Mar 28, 2017

Ch12 edits #576

Merged

carols10cents closed this Mar 28, 2017

carols10cents deleted the nostarch-ch12 branch May 16, 2017 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NOT TO MERGE comments from nostarch on ch12 for review #563

NOT TO MERGE comments from nostarch on ch12 for review #563

carols10cents commented Mar 25, 2017

carols10cents Mar 25, 2017

ScottAbbey Mar 25, 2017

ScottAbbey Mar 25, 2017

steveklabnik Mar 27, 2017

carols10cents Mar 25, 2017

carols10cents Mar 25, 2017

carols10cents Mar 25, 2017

steveklabnik Mar 27, 2017

carols10cents Mar 25, 2017

carols10cents Mar 25, 2017

carols10cents Mar 25, 2017

carols10cents Mar 25, 2017

carols10cents Mar 25, 2017

carols10cents Mar 28, 2017

carols10cents Mar 25, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

steveklabnik Mar 27, 2017

carols10cents commented Mar 28, 2017

		<!-- Will add ghosting and wingdings in libreoffice /Carol -->

		<!-- what does returning a Result rather than a Config do? -->

NOT TO MERGE comments from nostarch on ch12 for review #563

NOT TO MERGE comments from nostarch on ch12 for review #563

Conversation

carols10cents commented Mar 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carols10cents commented Mar 28, 2017