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

Separated argument parsing for serve subcommand. #625

Closed
wants to merge 5 commits into from

Conversation

maccoda
Copy link
Contributor

@maccoda maccoda commented Feb 17, 2018

Hopefully this can address the fourth point on #458. I wasn't sure if the struct notion for passing the arguments through was a good idea. Let me know. Also because of this to pass the struct into the closures it would need to implement Copy but this isn't simply derived as PathBuf does not implement it, hence the weird assignment of the variables before the closures. If there is a better way of handling this I would love to know, not entirely happy with that being the best option.

Thanks!

@Michael-F-Bryan
Copy link
Contributor

I was thinking a better solution overall would be to change argument parsing to structopt, that way we're cleaning up all the subcommands and not just one. This is essentially just a custom derive around clap which will configure a clap argument parser from your structures and ensure they get filled appropriately.

I've used structopt loads and find it avoids a lot of the annoying boilerplate associated with writing the clap parser and your own parse_args() function.

In this case we'd just have a top level enum containing all the subcommands and the arguments they expect.

For example:

#[derive(Debug, Clone, StructOpt)]
pub enum Command {
  #[structopt(name = "build", about = "Compile the document")]
  Build {
    #[structopt(short = "o", long = "short", help = "Open the compiled document")]
    open: bool,
  }
  #[structopt(name = "serve")]
  Serve {
    #[structopt(help = "The book's root directory", default_value = "."_]
    book_dir: PathBuf,
  }
  ...
}

@maccoda
Copy link
Contributor Author

maccoda commented Feb 18, 2018

@Michael-F-Bryan I thought there must have been something like that!! I will look into that one and have another go at it!

@maccoda
Copy link
Contributor Author

maccoda commented Jul 4, 2018

Hi @Michael-F-Bryan I think I have managed to get this one back in sync and hopefully ready to take. Let me know if I should change anything else. Thanks!

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.

2 participants