Add an option to specify the output directory - #104
Conversation
h4llow3En
changed the title
h4llow3En
changed the title
| let tmp = PathBuf::from(dir); | ||
| match tmp.extension() { | ||
| Some(_) => { | ||
| tt_warning!(status, "file extention in outdir; only the directory will be used"); |
There was a problem hiding this comment.
extention => extension
Maybe also outdir => `-o/--outdir`, so it's clear that outdir means the command line option?
| /// dirname of `primary_input_path`, or an empty path (i.e., corresponding | ||
| /// to the CWD if `primary_input_path` is None. | ||
| fs_root: PathBuf, | ||
| #[allow(unused)] fs_root: PathBuf, |
There was a problem hiding this comment.
Is #[allow(unused)] necessary?
There was a problem hiding this comment.
I put this in because now it is unused. But maybe it will be needed later?
Else there would be a warning while compiling.
There was a problem hiding this comment.
Oh, I see, I misunderstood, I thought the fs_root below is this one. I think that it is better to remove it. It's easy to add it back if needed.
There was a problem hiding this comment.
Okay, I will do this
| }}; | ||
| if !output_path.exists() { return Err(ErrorKind::Msg(format!("output directory \"{}\" does not exist", output_path.to_string_lossy())).into());} | ||
| } else { | ||
| output_path = fs_root.clone().to_path_buf() |
There was a problem hiding this comment.
Isn't this a double clone? Just clone() seems enough.
There was a problem hiding this comment.
Just clone() returns a &Path which is mismatched with PathBuf. Even if it should be equal because *PathBuf is a Path
| } | ||
| None => tmp, | ||
| }}; | ||
| if !output_path.exists() { return Err(ErrorKind::Msg(format!("output directory \"{}\" does not exist", output_path.to_string_lossy())).into());} |
|
|
||
| let mut real_path = self.fs_root.clone(); | ||
| let mut real_path = self.output_path.clone(); | ||
| real_path.push(name); |
There was a problem hiding this comment.
This can be written as
let real_path = self.output_path.join(name);
| let tempdir = setup_and_copy_files(&["subdirectory/content/1.tex"]); | ||
|
|
||
| let output = run_tectonic(tempdir.path(), | ||
| &["--format=plain.fmt.gz", "subdirectory/content/1.tex", "--outdir=subdirectory/non_existant"]); |
There was a problem hiding this comment.
non_existant => non_existent
| }}; | ||
| if !output_path.exists() { return Err(ErrorKind::Msg(format!("output directory \"{}\" does not exist", output_path.display())).into());} | ||
| } else { | ||
| output_path = fs_root.clone().to_path_buf() |
There was a problem hiding this comment.
I see, fs_root is Path. In that case, fs_root.to_path_buf() (or fs_root.to_owned()) should do.
| } | ||
|
|
||
| if let Some(dir) = args.value_of("outdir") { | ||
|
|
There was a problem hiding this comment.
Please remove this blank line.
| } | ||
| None => &tmp, | ||
| }}; | ||
| if !output_path.exists() { return Err(ErrorKind::Msg(format!("output directory \"{}\" does not exist", output_path.display())).into());} |
There was a problem hiding this comment.
Please don't condense the bodies of if statements to single lines. That is, please put the return statement and closing bracket on their own lines.
| .long("outdir") | ||
| .short("o") | ||
| .value_name("OUTDIR") | ||
| .help("The output directory. Default: Same like INPUT.")) |
There was a problem hiding this comment.
Please format the default description as it appears in the other arguments; something like
The directory in which to place output files. [default: the directory containing INPUT]
|
As you wished, I cleaned up the Code |
pkgw
left a comment
pkgw
left a comment
There was a problem hiding this comment.
Hi — I was looking over this PR again and I noticed a few more things in the handling of the argument that I think should be changed a bit. They're subtle but I think they're important for providing consistent behavior for the user. Can you please make these tweaks too? Thanks for taking the time to work through the process!
| } | ||
| } | ||
|
|
||
| if let Some(dir) = args.value_of("outdir") { |
There was a problem hiding this comment.
This should use value_of_os to get an OsString from the argument.
| if let Some(dir) = args.value_of("outdir") { | ||
| output_path = { | ||
| let tmp = Path::new(dir); | ||
| match tmp.extension() { |
There was a problem hiding this comment.
I don't think this code should special-case the circumstance where the argument path has an extension. It's not common but sometimes directory names have extensions like files, and in those cases you'll get surprising behavior because the program will alter the path that you pass it.
| None => &tmp, | ||
| } | ||
| }; | ||
| if !output_path.exists() { |
There was a problem hiding this comment.
On the other hand, I think here the check should use is_dir, not exists. I think this is closer to what we want and it will handle the case where the user inadvertently passes a filename instead of a directory name to the -o option.
|
I applied you change requests. Could you rerun the one failed test, please? |
|
Transient test failure resolved. Thanks! |
use objc crate's msg_send! macro to replace placeholders
3 participants
This closes #96.