Implement invocation_directory function (fix #312) #323

joshuawarner32 · 2018-06-16T16:09:01Z

invocation_directory retrieves the current directory that just was invoked from, before the cwd (current working directory) was changed to the location of the justfile.

The name's not perfect, but I'm not one for bikeshedding.

joshuawarner32 · 2018-06-16T16:11:16Z

src/function.rs

+  if let Some(dir) = context.invocation_directory {
+    Ok(dir.to_string())
+  } else {
+    Err(String::from("error getting invocation directory"))


I'm not super happy with this error message right now. The primary reason for this error would be an inability to decode the utf-8 of the original working directory, but it's not clear. Feedback/thoughts welcome.

I think that a really explicit error message is good, perhaps "invocation directory path not UTF-8, invocation-directory cannot be used"

Also, you can use Option::ok_or_else to turn None into an error.

Yeah - I don't disagree. The only thing that gives me pause is that where the error occurred is pretty far (in code) from where this string is - which makes me think that they could fall out of sync. For example, one day in the future there could be a different reason that invocation_directory can't be used, and it would be easy to forget to update this message.

What do you think about just having invocation_directory be passed along as a Result<String, String>? That way, the error string is constructed close to the error itself.

Alternatively, just could refuse to run when invoked from a non-utf8 path, but that seems rather draconian.

Good thinking. Passing invocation_directory as a ` Result<String, String> sounds like a good idea.

casey

Thank you very much for the PR! I added some comments inline.

We'll need a test for this. It should invoke the just binary with a recipe that prints out invocation_directory(), and then asserts that it prints the correct directory.

Since it needs to call the binary directly, it should be an integration test, so something like tests/invocation_directory.rs. If you look in tests/integration.rs, you'll see how to get the binary and path to the binary and invoke it.

Let me know if you have any questions!

casey · 2018-06-17T18:19:06Z

src/function.rs

+  if let Some(dir) = context.invocation_directory {
+    Ok(dir.to_string())
+  } else {
+    Err(String::from("error getting invocation directory"))


I think that a really explicit error message is good, perhaps "invocation directory path not UTF-8, invocation-directory cannot be used"

casey · 2018-06-17T18:23:34Z

src/run.rs

@@ -47,6 +47,11 @@ pub fn run() {
  #[cfg(windows)]
  enable_ansi_support().ok();

+  let invocation_directory = match env::current_dir() {
+    Ok(pathbuf) => pathbuf,


Let's do pathbuf.to_str().map(str::to_string) and then use Option<String> for the type of the invocation_directory value.

casey · 2018-06-17T18:28:12Z

src/function.rs

+  if let Some(dir) = context.invocation_directory {
+    Ok(dir.to_string())
+  } else {
+    Err(String::from("error getting invocation directory"))


Also, you can use Option::ok_or_else to turn None into an error.

joshuawarner32 · 2018-06-17T23:33:11Z

Hmm... I'm at a loss with what to do with the windows test. It seems to be a fundamental mismatch between windows paths and cygwin (or msys?) paths. Here are a few options I can think of:

Bow to the cygwin gods, and use conditional compilation (or just normal ifs) to conform to the style of paths that sh will expect on that system.
Only enable the test for *nix systems, where pathing like this is a little more sane
Pick a different shell to run this test under, that's going to deal better with windows paths. Perhaps cmd.exe?

casey · 2018-06-18T02:00:44Z

Hmmm, there's a utility called cygpath which just already uses to generate shebang paths on windows. (See src/platform.rs:55.) Could we use it here to translate the windows-style path to the cygwin path that the test will output, when we're running on windows?

casey · 2018-06-18T02:01:44Z

tests/invocation_directory.rs

+  justfile_path.push("justfile");
+  brev::dump(justfile_path, "default:\n @echo {{invocation_directory()}}");
+
+  let mut dotenv_path = tmp.path().to_path_buf();


I think the dotenv stuff can be left out of this test.

joshuawarner32 · 2018-06-19T14:50:06Z

Apologies for the CI noise on this PR; I don't have a great place to test on windows so I've been using CI.

I pivoted to providing the cygwin "unix" path on windows (rather than the native C:\... style path), which makes the tests pass and generally will make sh happier I think.

I think there's an argument to be made to expand invocation_directory() into the native path, or perhaps an escaped native path (i.e. C:\\..., doubling all the backslashes - which would also have made the tests pass in this case). In the future, that could be specified as e.g. invocation_directory("native") or invocation_directory("escaped"). However, I think that's out of scope for this PR, and definitely worth getting feedback from people who use just on Windows before heading that direction.

Anyway, do you have further thoughts/comments @casey?

…is usable

casey · 2018-06-19T17:05:43Z

That seems like the right thing to do, on windows recipes expect to be run in cygwin, so the unix path will be more immediately usable. I added the cd back into the test, as an extra test that the directory is in fact usable within recipes.

Nice work, and thanks for tackling what turned out to be much trickery issue than it first appeared!

casey · 2018-06-19T20:25:19Z

Just published the new release to crates.io; binaries will be available here once Travis and AppVeyor finish building them: https://github.com/casey/just/releases/tag/v0.3.12

Implement invocation_directory function (fix casey#312)

f30aef5

joshuawarner32 commented Jun 16, 2018

View reviewed changes

Add documentation section

36830dc

casey requested changes Jun 17, 2018

View reviewed changes

joshuawarner32 added 2 commits June 17, 2018 13:59

Add integration test for invocation_directory

7c354ae

Store invocation_directory as Result<String, String>

0dd135b

casey reviewed Jun 18, 2018

View reviewed changes

joshuawarner32 force-pushed the master branch 3 times, most recently from d60a287 to d58c98a Compare June 19, 2018 04:39

Attempt fixing windows test

4d1ea11

joshuawarner32 force-pushed the master branch from d58c98a to 4d1ea11 Compare June 19, 2018 04:41

Readmeformatting + cd into invocation_directory in test to ensure it …

6a9d90c

…is usable

casey approved these changes Jun 19, 2018

View reviewed changes

casey merged commit 50fb1b3 into casey:master Jun 19, 2018

casey mentioned this pull request Jun 19, 2018

Add function to return the directory just was invoked from #312

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement invocation_directory function (fix #312) #323

Implement invocation_directory function (fix #312) #323

joshuawarner32 commented Jun 16, 2018

joshuawarner32 Jun 16, 2018

casey Jun 17, 2018

casey Jun 17, 2018

joshuawarner32 Jun 17, 2018

casey Jun 17, 2018

casey left a comment

casey Jun 17, 2018

casey Jun 17, 2018

casey Jun 17, 2018

joshuawarner32 commented Jun 17, 2018

casey commented Jun 18, 2018

casey Jun 18, 2018

joshuawarner32 commented Jun 19, 2018

casey commented Jun 19, 2018

casey commented Jun 19, 2018

Implement invocation_directory function (fix #312) #323

Implement invocation_directory function (fix #312) #323

Conversation

joshuawarner32 commented Jun 16, 2018

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

casey 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

joshuawarner32 commented Jun 17, 2018

casey commented Jun 18, 2018

Choose a reason for hiding this comment

joshuawarner32 commented Jun 19, 2018

casey commented Jun 19, 2018

casey commented Jun 19, 2018