Implement `notion shim` command #100

mikrostew · 2018-07-20T22:26:56Z

Implements notion shim

List shims

Running notion shim lists the shims that exist in ~/.notion/bin:

$ notion shim
yarn
eslint
node
npx
npm

Running notion shim --verbose lists those shims and where they will resolve to:

# in a project where Notion is configured
$ notion shim -v
yarn -> /Users/mistewar/.notion/versions/yarn/1.2.0/bin/yarn
eslint -> /Users/mistewar/src/notion/crates/notion-core/fixtures/basic/node_modules/.bin/eslint
node -> /Users/mistewar/.notion/versions/node/6.11.1/bin/node
npx -> [shim not implemented!]
npm -> /Users/mistewar/.notion/versions/node/6.11.1/bin/npm

# when Notion is not configured
$ notion shim -v
yarn -> [system]
eslint -> [system]
node -> [system]
npx -> [shim not implemented!]
npm -> [system]

# when node or yarn has not been installed locally
$ notion shim -v
yarn -> [will install version = 1.3]
eslint -> /Users/mistewar/src/notion/crates/notion-core/fixtures/basic/node_modules/.bin/eslint
node -> [will install version = 6.11.3]
npx -> [shim not implemented!]
npm -> [will install version = 6.11.3]

Create shims

Running notion shim <name> creates a 3rd-party executable shim in the notion shim dir (~/.notion/bin/ on unix), pointing to .notion/launchbin:

$ notion shim foo
$ ls -og ~/.notion/bin/
total 22504
lrwxr-xr-x  1        33 Jul 23 13:11 foo -> /Users/mistewar/.notion/launchbin
lrwxr-xr-x  1        33 Jul 20 14:20 eslint -> /Users/mistewar/.notion/launchbin
-rwxr-xr-x  1   5946580 Jul 20 11:57 node
lrwxr-xr-x  1        36 Jul 20 11:57 npm -> /Users/mistewar/.notion/launchscript
lrwxr-xr-x  1        36 Jul 20 11:57 npx -> /Users/mistewar/.notion/launchscript
-rwxr-xr-x  1   5572172 Jul 20 11:57 yarn

# creating a shim that already exists shows an error
$ notion shim foo
error: shim `foo` already exists

Delete shims

Running notion shim <name> --delete deletes a 3rd-party executable shim:

$ ls -go ~/.notion/bin/
total 22504
lrwxr-xr-x  1        33 Jul 20 14:20 eslint -> /Users/mistewar/.notion/launchbin
lrwxr-xr-x  1        33 Jul 23 15:12 foo -> /Users/mistewar/.notion/launchbin
-rwxr-xr-x  1   5946580 Jul 20 11:57 node
lrwxr-xr-x  1        36 Jul 20 11:57 npm -> /Users/mistewar/.notion/launchscript
lrwxr-xr-x  1        36 Jul 20 11:57 npx -> /Users/mistewar/.notion/launchscript
-rwxr-xr-x  1   5572172 Jul 20 11:57 yarn

$ notion shim foo --delete
$ ls -go ~/.notion/bin/
total 22504
lrwxr-xr-x  1        33 Jul 20 14:20 eslint -> /Users/mistewar/.notion/launchbin
-rwxr-xr-x  1   5946580 Jul 20 11:57 node
lrwxr-xr-x  1        36 Jul 20 11:57 npm -> /Users/mistewar/.notion/launchscript
lrwxr-xr-x  1        36 Jul 20 11:57 npx -> /Users/mistewar/.notion/launchscript
-rwxr-xr-x  1   5572172 Jul 20 11:57 yarn

# errors if it doesn't exist, or you try to delete the non-3rd-party shims
$ notion shim foo --delete
error: shim `foo` does not exist
$ notion shim npm --delete
error: cannot delete `npm`, not a 3rd-party executable

Closes #96.

dherman

Looks good, just one request to refactor a deep function.

dherman · 2018-07-25T22:10:57Z

src/command/shim.rs

+    }
+}
+
+fn list(session: &mut Session, verbose: bool) -> Fallible<bool> {


This function is hard to read. Can you break out some of the nested loops into helper functions?

dherman

Some more feedback... hang in there, this is good progress and a good opportunity to pick up some good Rust style. We're getting there!

dherman · 2018-07-26T19:08:25Z

src/command/shim.rs

@@ -100,38 +108,41 @@ Options:
    }
 }

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


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

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!

dherman · 2018-07-26T19:10:03Z

src/command/shim.rs

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


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.

Doh! I missed that in the docs somehow

dherman · 2018-07-26T19:10:56Z

src/command/shim.rs

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


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.

dherman · 2018-07-26T19:11:21Z

src/command/shim.rs

        })
 }

+fn print_file_info(


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

So much better!

dherman · 2018-07-26T19:11:59Z

src/command/shim.rs

@@ -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>> {


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

Yeah I agree that's a better name

dherman

Looks great.

mikrostew force-pushed the shim-cmd branch from 26c7e4b to 03b044f Compare July 24, 2018 00:23

mikrostew changed the title ~~[WIP] Implement notion shim command~~ Implement notion shim command Jul 24, 2018

mikrostew requested review from dherman, monshi and benblank July 24, 2018 20:12

mikrostew added 7 commits July 25, 2018 10:00

implement 'shim create' command

70e4dd7

create shim

a855f3e

shim delete and extract shim things into a module

8249af1

fix exit codes

e5a7c88

rebase and rustfmt

ca99ee9

don't install node/yarn, show what will be installed

6e8f4d2

update for recent error changes

9e3cb2e

mikrostew force-pushed the shim-cmd branch from c3647f4 to 9e3cb2e Compare July 25, 2018 17:09

dherman requested changes Jul 25, 2018

View reviewed changes

refactor, add style, and don't install node

bf5965d

dherman requested changes Jul 26, 2018

View reviewed changes

simplify and fix error handling

2f5e3e1

dherman approved these changes Jul 26, 2018

View reviewed changes

dherman merged commit a64b114 into volta-cli:master Jul 26, 2018

dherman mentioned this pull request Jul 27, 2018

shimming for package executables #6

Closed

mikrostew deleted the shim-cmd branch December 13, 2018 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `notion shim` command #100

Implement `notion shim` command #100

mikrostew commented Jul 20, 2018 •

edited

Loading

dherman left a comment

dherman Jul 25, 2018

mikrostew Jul 26, 2018

dherman left a comment

dherman Jul 26, 2018

mikrostew Jul 26, 2018

dherman Jul 26, 2018

mikrostew Jul 26, 2018

dherman Jul 26, 2018

mikrostew Jul 26, 2018

dherman Jul 26, 2018

mikrostew Jul 26, 2018

dherman Jul 26, 2018

mikrostew Jul 26, 2018

dherman left a comment

Implement notion shim command #100

Implement notion shim command #100

Conversation

mikrostew commented Jul 20, 2018 • edited Loading

List shims

Create shims

Delete shims

dherman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dherman 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

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

Choose a reason for hiding this comment

dherman left a comment

Choose a reason for hiding this comment

Implement `notion shim` command #100

Implement `notion shim` command #100

mikrostew commented Jul 20, 2018 •

edited

Loading