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

Implement notion shim command #100

Merged
merged 9 commits into from
Jul 26, 2018
Merged

Implement notion shim command #100

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
70e4dd7
implement 'shim create' command
mikrostew Jul 20, 2018
a855f3e
create shim
mikrostew Jul 23, 2018
8249af1
`shim delete` and extract shim things into a module
mikrostew Jul 23, 2018
e5a7c88
fix exit codes
mikrostew Jul 24, 2018
ca99ee9
rebase and rustfmt
mikrostew Jul 24, 2018
6e8f4d2
don't install node/yarn, show what will be installed
mikrostew Jul 24, 2018
9e3cb2e
update for recent error changes
mikrostew Jul 25, 2018
bf5965d
refactor, add style, and don't install node
mikrostew Jul 26, 2018
2f5e3e1
simplify and fix error handling
mikrostew Jul 26, 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
108 changes: 67 additions & 41 deletions src/command/shim.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
use std::ffi::OsStr;
use std::fmt::{self, Display, Formatter};
use std::fs;
use std::path::PathBuf;
use std::{fs, io};

use console::style;
use notion_core::project::Project;
use notion_core::session::{ActivityKind, Session};
use notion_core::{path, shim, style};
use notion_fail::{Fallible, ResultExt};
use semver::VersionReq;
use semver::{Version, VersionReq};

use Notion;
use command::{Command, CommandName, Help};
Expand All @@ -29,6 +31,7 @@ enum ShimKind {
Local(PathBuf),
Global(PathBuf),
System,
NotInstalled,
WillInstall(VersionReq),
Unimplemented,
}
Expand All @@ -39,8 +42,13 @@ impl Display for ShimKind {
&ShimKind::Local(ref path) => format!("{}", path.to_string_lossy()),
&ShimKind::Global(ref path) => format!("{}", path.to_string_lossy()),
&ShimKind::System => format!("[system]"),
&ShimKind::NotInstalled => {
format!("{}", style("[executable not installed!]").red().bold())
}
&ShimKind::WillInstall(ref version) => format!("[will install version {}]", version),
&ShimKind::Unimplemented => format!("[shim not implemented!]"),
&ShimKind::Unimplemented => {
format!("{}", style("[shim not implemented!]").red().bold())
}
};
f.write_str(&s)
}
Expand Down Expand Up @@ -100,38 +108,41 @@ Options:
}
}

fn list(session: &mut Session, verbose: bool) -> Fallible<bool> {
fn list(session: &Session, verbose: bool) -> Fallible<bool> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an improvement! But there are still a few things we can do to make it simpler. First a few general points:

  • You should generally avoid inspecting error cases, and just propagate them.
  • You should generally prefer let foo = bar()?; to bar().map(|foo|, { ... }). This avoids rightward drift, avoids confusing iteration map versus result map, and lets you code closer to a style that's like exceptions. As an analogy in JS, this is why let foo = await bar(); is nicer than bar.then((foo) => { ... }).
  • It's easiest to just convert foreign error types like io::Error into notion's error types as early as possible, and the ? syntax makes that lightweight. So instead of returning Result<Foo, io::Result> from helper functions, just return Fallible<Foo>.

As for the Command trait, don't throw away errors and return Ok(false) -- you want to let the particular error propagate out.

Here's how we could write this function to be simpler and more readable/maintainable:

fn list(session: &Session, verbose: bool) -> Fallible<()> {
    let shim_dir = path:shim_dir()?;
    let files = fs::read_dir(shim_dir).unknown()?;

    for file in files {
        print_file_info(f, session, verbose)?;
    }

    Ok(())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is so much simpler!

I made peace with the borrow checker, but lately I have been fighting Result and Option. This helps a lot!

path::shim_dir()
.and_then(|shim_dir| fs::read_dir(shim_dir).unknown())
.map(|files| {
files
.map(|file| {
file.and_then(|f| {
f.path().file_name().map_or(Ok(false), |shim_name| {
if verbose {
match resolve_shim(session, &shim_name) {
Ok(shim_info) => {
println!("{} -> {}", shim_name.to_string_lossy(), shim_info)
},
Err(err) => {
style::display_error(style::ErrorContext::Notion, &err);
return Ok(false);
},
}
} else {
println!("{}", shim_name.to_string_lossy());
}
Ok(true)
})
})
})
.collect::<Vec<_>>()
.iter()
// return false if anything failed
.all(|ref result| result.as_ref().ok() == Some(&true))
files.map(|file| {
file.and_then(|f| print_file_info(f, session, verbose))
})
.collect::<Vec<_>>()
.iter()
// return false if anything failed
.all(|ref result| result.as_ref().ok() == Some(&true))
})
}

fn print_file_info(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here's a simplified version of this function:

fn print_file_info(file: fs::DirEntry, session: &Session, verbose: bool) -> Fallible<()> {
    let shim_name = file.file_name();

    if verbose {
        let shim_info = resolve_shim(session, &shim_name)?;
        println!("{} -> {}", shim_name.to_string_lossy(), shim_info);
    } else {
        println!("{}", shim_name.to_string_lossy());
    }

    Ok(())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So much better!

file: fs::DirEntry,
session: &Session,
verbose: bool,
) -> Result<bool, io::Error> {
file.path().file_name().map_or(Ok(false), |shim_name| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

A DirEntry always has a file name, so instead of using the Option that comes from file.path().file_name() you can use file.file_name() which directly returns an OsString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh! I missed that in the docs somehow

if verbose {
match resolve_shim(session, &shim_name) {
Ok(shim_info) => println!("{} -> {}", shim_name.to_string_lossy(), shim_info),
Err(err) => {
style::display_error(style::ErrorContext::Notion, &err);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we want to be doing this error handling logic here, as opposed to letting it propagate out and be handled at the top level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

return Ok(false);
}
}
} else {
println!("{}", shim_name.to_string_lossy());
}
Ok(true)
})
}

fn create(_session: &Session, shim_name: String, _verbose: bool) -> Fallible<bool> {
shim::create(&shim_name)?;
Ok(true)
Expand All @@ -142,7 +153,7 @@ fn delete(_session: &Session, shim_name: String, _verbose: bool) -> Fallible<boo
Ok(true)
}

fn resolve_shim(session: &mut Session, shim_name: &OsStr) -> Fallible<ShimKind> {
fn resolve_shim(session: &Session, shim_name: &OsStr) -> Fallible<ShimKind> {
match shim_name.to_str() {
Some("node") | Some("npm") => resolve_node_shims(session, shim_name),
Some("yarn") => resolve_yarn_shims(session, shim_name),
Expand All @@ -152,13 +163,18 @@ fn resolve_shim(session: &mut Session, shim_name: &OsStr) -> Fallible<ShimKind>
}
}

fn node_is_available(project: &Project, session: &Session) -> Fallible<Option<Version>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this isn't a boolean predicate, maybe a better name would be something like available_node_version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree that's a better name

let requirements = &project.manifest().node;
let catalog = session.catalog()?;
Ok(catalog.node.resolve_local(&requirements))
}

// figure out which version of node is installed or configured,
// or which version will be installed if it's not available locally
fn resolve_node_shims(session: &Session, shim_name: &OsStr) -> Fallible<ShimKind> {
if let Some(project) = session.project() {
let requirements = &project.manifest().node;
let catalog = session.catalog()?;
if let Some(available) = catalog.node.resolve_local(&requirements) {
if let Some(available) = node_is_available(&project, &session)? {
// node is available locally - this shim will use that version
let mut bin_path = path::node_version_bin_dir(&available.to_string()).unknown()?;
bin_path.push(&shim_name);
Expand Down Expand Up @@ -201,25 +217,35 @@ fn resolve_yarn_shims(session: &Session, shim_name: &OsStr) -> Fallible<ShimKind
Ok(ShimKind::System)
}

fn resolve_npx_shims(_session: &mut Session, _shim_name: &OsStr) -> Fallible<ShimKind> {
fn resolve_npx_shims(_session: &Session, _shim_name: &OsStr) -> Fallible<ShimKind> {
Ok(ShimKind::Unimplemented)
}

fn resolve_3p_shims(session: &mut Session, shim_name: &OsStr) -> Fallible<ShimKind> {
// if this is a local executable, get the path to that
fn resolve_3p_shims(session: &Session, shim_name: &OsStr) -> Fallible<ShimKind> {
if let Some(project) = session.project() {
// if this is a local executable, get the path to that
if project.has_local_bin(shim_name)? {
let mut path_to_bin = project.local_bin_dir();
path_to_bin.push(shim_name);
return Ok(ShimKind::Local(path_to_bin));
}

// if node is installed, use the bin there
if let Some(available) = node_is_available(&project, &session)? {
// node is available locally - this shim will use that version
let mut bin_path = path::node_version_3p_bin_dir(&available.to_string())?;
bin_path.push(&shim_name);
return Ok(ShimKind::Global(bin_path));
}
// if node is not installed, this shim has not been installed for this node version
return Ok(ShimKind::NotInstalled);
}
// if node is configured with Notion, use the global executable
// if node is globally configured with Notion, use the global executable
// otherwise it's a shim to system executables
let version = session.current_node()?;
version.map_or(Ok(ShimKind::System), |v| {
let mut third_p_bin_dir = path::node_version_3p_bin_dir(&v.to_string())?;
third_p_bin_dir.push(&shim_name);
Ok(ShimKind::Global(third_p_bin_dir))
let global_version = session.global_node()?;
global_version.map_or(Ok(ShimKind::System), |gv| {
let mut bin_path = path::node_version_3p_bin_dir(&gv.to_string())?;
bin_path.push(&shim_name);
Ok(ShimKind::Global(bin_path))
})
}