Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add chapter on commandline arguments #76

Merged
merged 1 commit into from
Dec 11, 2014
Merged

Conversation

vks
Copy link
Contributor

@vks vks commented Jun 4, 2014

Please let me know if you think the example should be more verbose.

fn main() {
let args = os::args();
println!("My path is {}.", args.get(0));
println!("My arguments are {}.", args.slice(1, args.len()));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we could use tail() instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about tail, thanks!

(I'm surprised that this is in the standard library, seems very specialized to me. Do you know whether it was ever considered to add something more generic like Python's slicing syntax?)

@japaric
Copy link
Member

japaric commented Jun 5, 2014

Thanks for sending a PR!

This is a very succinct chapter :P. I would like to see more things (if you have the time, otherwise I can take over):

  • Show the type of the value returned by os::args(), this can be done with a superfluous type annotation.
  • Show how to get the number of arguments: args.len(). (this could be added to the first example)
  • Show how to do pattern matching on the cmd arguments, something like
let args = os::args();

match args.as_slice() {
    // no arguments passed
    [ref name] => {
        // some behavior
    },
    // one argument passed
    [ref name, ref num] => {
        // call from_str on `num`
        // another behavior
    },
    // one command and one argument passed
    [ref name, ref cmd, ref num] => {
        match cmd.as_slice() {
            "action1" => {},
            "action2" => {},
            _ => help(),
        }
    },
    // all the other cases
    _ => {
        // show a help message
        help();
    }
}

(^ that should be a second example)

  • mention that more complex argument parsing can be done with getopts, which will be covered in another chapter.

@vks
Copy link
Contributor Author

vks commented Jun 6, 2014

Thanks for the feedback! I made some according changes.

The second example became a bit long.

I accidentally committed changes to examples/order.json, is this generated by the build system? If yes, it probably should not be tracked by git.

@japaric japaric mentioned this pull request Jun 7, 2014
@japaric
Copy link
Member

japaric commented Jun 7, 2014

I think we should put this on hold, until #80 is finished.

After that lands, we should split this chapter like this:

Program arguments
|-- Reading the arguments
|-- The match idiom
`-- `getopts``

(name subject to changes)

What do you think about the split? We could leave the getopts subchapter empty for now.

@vks
Copy link
Contributor Author

vks commented Jun 7, 2014

I think the split makes sense. More structure is good.

2014-06-07 12:12 GMT-04:00 Jorge Aparicio notifications@github.com:

I think we should put this on hold, until #80
#80 is finished.

After that lands, we should split this chapter like this:

Program arguments
|-- Reading the arguments
|-- The match idiom
--getopts``

(name subject to changes)

What do you think about the split? We could leave the getopts subchapter
empty for now.


Reply to this email directly or view it on GitHub
#76 (comment)
.

@japaric
Copy link
Member

japaric commented Jun 14, 2014

This needs a rebase

EDIT: I just had an idea, maybe we can make a "CLI" chapter, and include "args" as a subchapter, and the subchapters could be: "exit code", "stdin", "stdout" and "stderr". What do you think?

@japaric
Copy link
Member

japaric commented Jun 17, 2014

cc #106

@vks
Copy link
Contributor Author

vks commented Jun 20, 2014

Rebased and split into subchapters. I added an implementation of echo as a getopts example.

There seems to be an issue with the syntax highlighting of the last example. Also it breaks the playpen, probably because it uses libc::exit.

I would love to have some input on the second example, I'm not very happy with it yet.

@japaric I agree about the CLI chapter, especially since my last example (echo) uses exit codes.

@@ -0,0 +1,71 @@
/*
* For the full copyright and license information, please view the LICENSE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would look odd, because none of the other examples include a header comment. Maybe we can add a note after the example to indicate that this example is based on coreutils's echo code.

@japaric
Copy link
Member

japaric commented Jun 30, 2014

Sorry for procrastinating this for so long :-(.

There seems to be an issue with the syntax highlighting of the last example.

Yes, this is related to issue #2. This problem doesn't happen with the live code editor.

Also it breaks the playpen, probably because it uses libc::exit.

Your original program works fine in the playpen.

I would love to have some input on the second example, I'm not very happy with it yet.

Except for the code style issues, I think the coverage of features is good. Does something else bother you?

I agree about the CLI chapter, especially since my last example (echo) uses exit codes.

We can merge this as a chapter for now, and later move it into the CLI chapter as a subchapter.

@vks
Copy link
Contributor Author

vks commented Jul 1, 2014

@japaric I think I addressed all of your comments.

Does something else bother you?

I think the coverage is fine, I'm just not a big fan of the silly example, but I don't have a better idea so far.

Currently there is a very strange bug, where the getopts chapter shows up as a subchapter of the random chapter.

@@ -0,0 +1,64 @@
extern crate getopts;
extern crate libc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this anymore, right?


use std::os;
use std::io::{print, println};
use std::io::stdio::flush;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idiomatic way to use functions from other modules is to import the module use std::io::stdio, and call the function using the path: stdio::flush().

@japaric
Copy link
Member

japaric commented Jul 1, 2014

@japaric I think I addressed all of your comments.

Sorry, I have more comments :P

I think the coverage is fine, I'm just not a big fan of the silly example, but I don't have a better idea so far.

It just occurred to me (although it may be too late) that you could do use echo as a base for the three examples, making it "evolve" in complexity as one progresses through the examples.

First example: just "connect" all the arguments, and print the result. No flag parsing.
Second example: Parse certain flags, but position of the flags is important (this may be tricky)
Third example: Parsing flags regardless of their position.

Of course, this is more work, and the current examples are already good.

Currently there is a very strange bug, where the getopts chapter shows up as a subchapter of the random chapter.

You are right, I see the same thing. That's really weird! I'll have to investigate.

@vks
Copy link
Contributor Author

vks commented Jul 1, 2014

First example: just "connect" all the arguments, and print the result. No
flag parsing.
Second example: Parse certain flags, but position of the flags is
important (this may be tricky)
Third example: Parsing flags regardless of their position.

This would be a nicer example but cover less:

  • In the first example it would not mention the 0th argument to os::args
    (which I think is crucial for a first example).
  • Parsing non-strings (here: integers) would not be covered at all, which
    is important, because it is not that obvious in Rust (because of Option).

2014-06-30 21:25 GMT-04:00 Jorge Aparicio notifications@github.com:

@japaric /~https://github.com/japaric I think I addressed all of your
comments.

Sorry, I have more comments :P

I think the coverage is fine, I'm just not a big fan of the silly example,
but I don't have a better idea so far.

It just occurred to me (although it may be too late) that you could do use
echo as a base for the three examples, making it "evolve" in complexity
as one progresses through the examples.

First example: just "connect" all the arguments, and print the result. No
flag parsing.
Second example: Parse certain flags, but position of the flags is
important (this may be tricky)
Third example: Parsing flags regardless of their position.

Of course, this is more work, and the current examples are already good.

Currently there is a very strange bug, where the getopts chapter shows up
as a subchapter of the random chapter.

You are right, I see the same thing. That's really weird! I'll have to
investigate.


Reply to this email directly or view it on GitHub
#76 (comment)
.

@japaric
Copy link
Member

japaric commented Jul 1, 2014

This would be a nicer example but cover less:

Oh well, at least I tried :P

@japaric
Copy link
Member

japaric commented Jul 2, 2014

@vks Can getopts handle arguments like:

  • --type f and --out-dir bin, where the argument after the flag is like an "input" - this could be translated to a map: "type" -> "f" and "out-dir" -> "bin" in the latter case.
  • or --type=f and --out-dir=bin, with a similar treatment to the previous case?

@vks
Copy link
Contributor Author

vks commented Jul 2, 2014

@japaric Yes, you can use optopt instead of optflag.

You can use equivalently

$ command --type=f
$ command --type f
$ command -t=f
$ command -t f
$ command -tf

(Judging from the documentation, I did not actually check that.)

Do you think we should add an example for this?

@japaric
Copy link
Member

japaric commented Jul 3, 2014

Do you think we should add an example for this?

Yes, that'd be awesome. BTW, the weird bug (getopts example under random chapter) seems to have died with the last commit.

@vks
Copy link
Contributor Author

vks commented Jul 3, 2014

I might add an example for optional options, but this will be in a separate pull request, because I need to think of a new example (echo cannot be easily extended for this).

@vks
Copy link
Contributor Author

vks commented Aug 27, 2014

@japaric What's the status on this? Anything that still needs to be addressed?

@steveklabnik
Copy link
Member

@vks in general, I am interested in adding a chapter like this, but this PR is very old. Do you have any interest in modernizing it? If so, let's get it finally shipped.

@vks
Copy link
Contributor Author

vks commented Dec 10, 2014

@steveklabnik I will rebase and update the code.

@vks
Copy link
Contributor Author

vks commented Dec 10, 2014

@steveklabnik This should work with a current version of Rust now.

I think japaric reviewed everything except for the getopts example.

@steveklabnik
Copy link
Member

I think that overall this looks great, but I'm loathe to add another license. If we address that and squash this up, r=me :)

@vks
Copy link
Contributor Author

vks commented Dec 10, 2014

I'll ask the uutils maintainers just to be sure. Do you want me to squash
everything into one commit?

On Wed Dec 10 2014 at 2:10:23 PM Steve Klabnik notifications@github.com
wrote:

I think that overall this looks great, but I'm loathe to add another
license. If we address that and squash this up, r=me :)


Reply to this email directly or view it on GitHub
#76 (comment)
.

@steveklabnik
Copy link
Member

Yes please.

@steveklabnik
Copy link
Member

This is out of date already O_O

@vks
Copy link
Contributor Author

vks commented Dec 11, 2014

Should be fixed now. (The conflict was a good thing, because I forgot to remove the special mention of the license from the readme.)

Regarding the license, we got green light by Seldaek, who is the copyright holder in uutils' license.

steveklabnik added a commit that referenced this pull request Dec 11, 2014
Add chapter on commandline arguments
@steveklabnik steveklabnik merged commit 9e84f2d into rust-lang:master Dec 11, 2014
@steveklabnik
Copy link
Member

Thank you!

@vks vks deleted the args branch December 11, 2014 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants