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.

Sign up for GitHub

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

#340 Allow justfile without workdir #355

Closed
wants to merge 9 commits into from
Closed

#340 Allow justfile without workdir #355

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
65e3a53
Allow without
smonami Sep 13, 2018
3c345b4
making --working-directory optional for --justfile and --edit
smonami Sep 17, 2018
45512fd
Merge branch 'master' into allow_justfile_without_workdir
smonami Sep 17, 2018
5adf44c
#340 Allow justfile without workdir
smonami Sep 21, 2018
72772ad
Merge branch 'master' into allow_justfile_without_workdir
smonami Sep 22, 2018
07fa147
#340 derive absolute path for working_directory_option
smonami Oct 18, 2018
6cb7eaa
Merge branch 'master' into allow_justfile_without_workdir
smonami Oct 19, 2018
119b798
Refactor to use path and pathbuf for justfile and working directory
casey Oct 25, 2018
7f41ec4
#340 integration test for making --WORKING-DIRECTORY optional
smonami Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/run.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use common::*;

use std::{convert, ffi};
use std::{convert, ffi };
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like this change isn't needed.

Copy link
Author

Choose a reason for hiding this comment

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

Corrected extra space.

use clap::{App, Arg, ArgGroup, AppSettings};
use configuration::DEFAULT_SHELL;
use misc::maybe_s;
Expand Down Expand Up @@ -86,8 +86,7 @@ pub fn run() {
.short("f")
.long("justfile")
.takes_value(true)
.help("Use <JUSTFILE> as justfile. --working-directory must also be set")
.requires("WORKING-DIRECTORY"))
.help("Use <JUSTFILE> as justfile. --working-directory can be set [default: supplied justfile location]"))
Copy link
Owner

Choose a reason for hiding this comment

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

I think that since the behavior of just in setting the working directory is the same as normal when using this flag, we can make the comment shorter. Just Use <JUSTFILE> as justfile. is fine.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

.arg(Arg::with_name("LIST")
.short("l")
.long("list")
Expand Down Expand Up @@ -187,7 +186,12 @@ pub fn run() {
.collect::<Vec<&str>>();

let justfile_option = matches.value_of("JUSTFILE");
let working_directory_option = matches.value_of("WORKING-DIRECTORY");
let mut working_directory_option = matches.value_of("WORKING-DIRECTORY");
Copy link
Owner

Choose a reason for hiding this comment

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

Let's change working_directory_option to a Option<&Path>, so that we're working with paths from here on out, and don't have to convert to a string. You can do .map(Path::new).

Copy link
Author

Choose a reason for hiding this comment

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

Done.


if justfile_option.is_some() && working_directory_option.is_none() {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's change this to an if let, so we can avoid the unwraps:

if let (Some(justfile), None) = (justfile_option, working_directory_option) {

Copy link
Author

Choose a reason for hiding this comment

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

Done.

let justfile_path = Path::new(justfile_option.unwrap()).parent();
Copy link
Owner

Choose a reason for hiding this comment

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

If parent() returns none, we should exit with a message, like die!("Could not find parent directory of justfile at {}", justfile)

Copy link
Author

Choose a reason for hiding this comment

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

Added none check. But there seems to be a known issue(rust-lang/rust#36861) with parent returning "" in certain cases. Please suggest if it can be handled in a better way.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the changes! Let's resolve the justfile to an absolute path, and then using the parent of that.

Copy link
Owner

Choose a reason for hiding this comment

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

That should fix the issue with parent returning ""

Copy link
Author

Choose a reason for hiding this comment

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

Disclaimer: I am very new to Rust :-), and although I understand reference concept, not able to find a 'Rust way' to deal with following situation and so having trouble with the code:

if let (Some(justfile), None) = (justfile_option, working_directory_option) {
     let justfile_path = Path::new(justfile);
    if Path::new(justfile).is_absolute() {
        working_directory_option = Path::new(justfile).parent().unwrap().to_str();
    } else {
      let justfile_path_canonical = justfile_path.canonicalize();
      if justfile_path.canonicalize().is_ok() {
// @casey following line errors out with borrowed value does not live long enough. Any suggestion?
        working_directory_option = justfile_path_canonical.ok().unwrap().to_str(); 
      } else {
        die!("Could not find parent directory of justfile at {}.", justfile);
      }
    }
    //println!("working_directory_option:{:?}", working_directory_option);
  }

Copy link
Owner

Choose a reason for hiding this comment

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

No worries at all, we were all new to Rust once :)

Can you push the code (even if it's broken) to the PR branch so I can try it out?

Copy link
Author

Choose a reason for hiding this comment

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

Done. It's broken.

working_directory_option = justfile_path.unwrap().to_str();
}

let text;
if let (Some(file), Some(directory)) = (justfile_option, working_directory_option) {
Expand Down