Add an option to specify the output directory #104

h4llow3En · 2017-06-16T06:04:15Z

This closes #96.

rekka · 2017-06-16T22:50:49Z

src/cli_driver.rs

+                let tmp = PathBuf::from(dir);
+                match tmp.extension() {
+                    Some(_) => {
+                        tt_warning!(status, "file extention in outdir; only the directory will be used");


extention => extension
Maybe also outdir => `-o/--outdir`, so it's clear that outdir means the command line option?

rekka · 2017-06-16T22:55:36Z

src/cli_driver.rs

@@ -363,7 +363,7 @@ struct ProcessingSession {
    /// This is the virtual "CWD" that our filesystem accesses use. It is the
    /// 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,


Is #[allow(unused)] necessary?

I put this in because now it is unused. But maybe it will be needed later?
Else there would be a warning while compiling.

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.

Okay, I will do this

rekka · 2017-06-16T22:58:19Z

src/cli_driver.rs

+                }};
+            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()


Isn't this a double clone? Just clone() seems enough.

Just clone() returns a &Path which is mismatched with PathBuf. Even if it should be equal because *PathBuf is a Path

rekka · 2017-06-16T22:58:47Z

src/cli_driver.rs

+                        }
+                    None => tmp,
+                }};
+            if !output_path.exists() { return Err(ErrorKind::Msg(format!("output directory \"{}\" does not exist", output_path.to_string_lossy())).into());}


to_string_lossy => display

rekka · 2017-06-16T23:02:50Z

src/cli_driver.rs

@@ -692,9 +714,10 @@ impl ProcessingSession {
                continue;
            }

-            let mut real_path = self.fs_root.clone();
+            let mut real_path = self.output_path.clone();
            real_path.push(name);


This can be written as

let real_path = self.output_path.join(name);

rekka · 2017-06-16T23:06:23Z

tests/executable.rs

+    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"]);


non_existant => non_existent

rekka · 2017-06-17T12:52:54Z

src/cli_driver.rs

+                }};
+            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()


I see, fs_root is Path. In that case, fs_root.to_path_buf() (or fs_root.to_owned()) should do.

pkgw

Sorry it took me so long to get to this! Thanks, looks good — I just have a few small stylistic requests.

pkgw · 2017-06-20T02:02:03Z

src/cli_driver.rs

                io_builder.filesystem_root(par);
            } else {
                return Err(ErrorKind::Msg(format!("can't figure out a parent directory for input path \"{}\"",
                                                  tex_path.to_string_lossy())).into());
            }
        }

+        if let Some(dir) = args.value_of("outdir") {
+


Please remove this blank line.

pkgw · 2017-06-20T02:02:39Z

src/cli_driver.rs

+                        }
+                    None => &tmp,
+                }};
+            if !output_path.exists() { return Err(ErrorKind::Msg(format!("output directory \"{}\" does not exist", output_path.display())).into());}


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.

pkgw · 2017-06-20T02:07:29Z

src/cli_driver.rs

+             .long("outdir")
+             .short("o")
+             .value_name("OUTDIR")
+             .help("The output directory. Default: Same like INPUT."))


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]

h4llow3En · 2017-06-20T07:08:30Z

As you wished, I cleaned up the Code

pkgw

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!

pkgw · 2017-06-21T12:16:53Z

src/cli_driver.rs

                io_builder.filesystem_root(par);
            } else {
                return Err(ErrorKind::Msg(format!("can't figure out a parent directory for input path \"{}\"",
                                                  tex_path.to_string_lossy())).into());
            }
        }

+        if let Some(dir) = args.value_of("outdir") {


This should use value_of_os to get an OsString from the argument.

pkgw · 2017-06-21T12:19:20Z

src/cli_driver.rs

+        if let Some(dir) = args.value_of("outdir") {
+            output_path = {
+                let tmp = Path::new(dir);
+                match tmp.extension() {


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.

pkgw · 2017-06-21T12:21:20Z

src/cli_driver.rs

+                    None => &tmp,
+                }
+            };
+            if !output_path.exists() {


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.

h4llow3En · 2017-06-21T14:13:34Z

I applied you change requests. Could you rerun the one failed test, please?

pkgw · 2017-06-21T15:26:21Z

Transient test failure resolved. Thanks!

use objc crate's msg_send! macro to replace placeholders

h4llow3En added 2 commits June 16, 2017 08:00

add -o, --outdir option

c745ce6

chose output name

dd3fef9

h4llow3En changed the title ~~Add an option to specify the output directory~~ WIP: Add an option to specify the output directory Jun 16, 2017

h4llow3En and others added 3 commits June 16, 2017 11:29

fixed tests, add check if outdir exists

d79bccb

Merge branch 'master' into master

d720eaf

fix merge

5aa0c49

h4llow3En changed the title ~~WIP: Add an option to specify the output directory~~ Add an option to specify the output directory Jun 16, 2017

rekka reviewed Jun 16, 2017

View reviewed changes

Fix typos and minor improvements

fbcfd7a

rekka reviewed Jun 17, 2017

View reviewed changes

pkgw reviewed Jun 20, 2017

View reviewed changes

Remove unused fs_root and cleanup

228658a

pkgw reviewed Jun 21, 2017

View reviewed changes

Improve outdir checking

c2dc448

pkgw merged commit 6761ceb into tectonic-typesetting:master Jun 21, 2017

Mrmaxmeier pushed a commit to Mrmaxmeier/tectonic that referenced this pull request Oct 1, 2019

Merge pull request tectonic-typesetting#104 from cormacrelf/objc_msgSend

72da158

use objc crate's msg_send! macro to replace placeholders

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to specify the output directory #104

Add an option to specify the output directory #104

h4llow3En commented Jun 16, 2017

rekka Jun 16, 2017

rekka Jun 16, 2017

h4llow3En Jun 17, 2017

rekka Jun 17, 2017

h4llow3En Jun 17, 2017

rekka Jun 16, 2017

h4llow3En Jun 17, 2017 •

edited

Loading

rekka Jun 16, 2017

rekka Jun 16, 2017

rekka Jun 16, 2017

rekka Jun 17, 2017

pkgw left a comment •

edited

Loading

pkgw Jun 20, 2017

pkgw Jun 20, 2017

pkgw Jun 20, 2017

h4llow3En commented Jun 20, 2017

pkgw left a comment

pkgw Jun 21, 2017

pkgw Jun 21, 2017

pkgw Jun 21, 2017

h4llow3En commented Jun 21, 2017

pkgw commented Jun 21, 2017

Add an option to specify the output directory #104

Add an option to specify the output directory #104

Conversation

h4llow3En commented Jun 16, 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

h4llow3En Jun 17, 2017 • edited Loading

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

pkgw left a comment • edited Loading

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

h4llow3En commented Jun 20, 2017

pkgw left a 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

h4llow3En commented Jun 21, 2017

pkgw commented Jun 21, 2017

h4llow3En Jun 17, 2017 •

edited

Loading

pkgw left a comment •

edited

Loading