Improve UX for pack and publish path expectations #198

Mackiovello · 2018-07-04T18:06:30Z

Okay, I have to take this piece by piece. I'm creating this as a WIP pull request to some pressure on myself but also to make it easier to ask for help.

If anyone sees anything that can be improved or think should be changed, please tell me. I'm new to Rust, so every little hint helps. I'll put some questions in my own code for my own reference but also for you to answer if you have time.

This is for #189

saving this for later

Make sure these boxes are checked! 📦✅

You have the latest version of rustfmt installed and have your
cloned directory set to nightly

$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly

You ran rustfmt on the code base before submitting
You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

Mackiovello · 2018-07-04T18:07:29Z

src/command/pack.rs

-    npm::npm_pack(&crate_path)?;
+    match npm::npm_pack(&crate_path) {
+        Ok(r) => r,
+        Err(Error::Io { .. }) => {


I think this match is too broad. I only want to catch os error 2: directory not found. I don't want to swallow other IO errors. Investigate how to do that.

Mackiovello · 2018-07-04T18:08:11Z

src/command/pack.rs

@@ -9,7 +9,15 @@ pub fn pack(path: Option<String>, log: &Logger) -> result::Result<(), Error> {
    let crate_path = set_crate_path(path);

    info!(&log, "Packing up the npm package...");
-    npm::npm_pack(&crate_path)?;
+    match npm::npm_pack(&crate_path) {
+        Ok(r) => r,


This should probably just be Ok(r) => (),

Mackiovello · 2018-07-04T18:09:07Z

src/command/pack.rs

+        Ok(r) => r,
+        Err(Error::Io { .. }) => {
+            return Err(Error::DirNotFound {
+                message: "Unable to find the pkg directory".to_owned(),


Should this be more specific? Should it also print the path that the user entered? Not sure yet.

Mackiovello · 2018-07-04T18:09:53Z

src/command/pack.rs

+                message: "Unable to find the pkg directory".to_owned(),
+            });
+        }
+        Err(e) => return Err(e),


These explicit return statements feel like code smells. Maybe I can use the ? operator here?

I should try something like this instead:

my_function_call().map_err(|e| match { interesting_error => something_else, stuff => stuff, })?

Mackiovello · 2018-07-05T17:48:17Z

@mgattozzi could you give this a quick look? Am I on the right path?

Also, how should I do with tests? I don't like shipping code without unit tests. It looks like I have to inject the npm functions in order to test. Is there a better way as you see it?

The code is repeated between pack and publish. Do you believe it should be abstracted or do we keep it like this?

This lets us have backwards compatibility while improving the user experience.

ashleygwilliams · 2018-07-10T13:52:19Z

thanks so much for this @Mackiovello ! i'll give a review by EOD today :)

Mackiovello · 2018-07-14T05:05:48Z

This does not work as expected right now. I'll look at it later today. Don't review yet

…ssue/189

Mackiovello · 2018-07-14T10:31:47Z

Never mind, it actually works. I was confused since I got os error 2, but that was because I didn't have NPM installed. Here's how it works with NPM installed:

$ cargo run -- pack myproject/pkg
   Compiling wasm-pack v0.4.1 (file:///home/erlend/rust/wasm-pack)
    Finished dev [unoptimized + debuginfo] target(s) in 2.46s
     Running `target/debug/wasm-pack pack myproject/pkg`

| 🎒  packed up your package!
$ cargo run -- pack myproject
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/wasm-pack pack myproject`

| 🎒  packed up your package!
$ cargo run -- pack myproject/src/
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/wasm-pack pack myproject/src/`
Unable to find the pkg directory at path 'myproject/src/', or in a child directory of 'myproject/src/'
erlend@mackiovello:~/rust/wasm-pack$

Mackiovello · 2018-07-14T10:59:30Z

Ran cargo +nightly fmt with no result:

$ cargo +nightly fmt
$ git status
On branch issue/189
Your branch is up-to-date with 'origin/issue/189'.
nothing to commit, working directory clean

Mackiovello · 2018-07-14T11:41:10Z

@ashleygwilliams this is ready for a review 🙂

ashleygwilliams · 2018-07-16T14:14:46Z

hey @Mackiovello ! thanks so much for this. so far it looks great! do you think you could add some docs and perhaps a test (let me know if you run into trouble with this one)? since this is about expectations, i think some docs would be great :) should be close to getting this in tho- thanks so much again!

ashleygwilliams

needs docs and a test! once that's done i'm happy to merge

Mackiovello · 2018-07-16T14:22:00Z

Thank you @ashleygwilliams! One question: Where do you want documentation? On the pack and publish functions or somewhere else?

Also, should the tests be unit tests with no access to the file system or should it be integration tests that are allowed to use the file system? I think it should be fine with unit tests, but then I have to inject functions like is_dir and mock it in the tests. It'll make the code a little messier.

ashleygwilliams · 2018-07-16T14:35:46Z

@Mackiovello so- for the docs, i would add both API docs (not too specific to end user, just add descriptions of the functionality generally), and then and more user-facing specific docs to the docs in the docs dir. looks like pack and publish are separate at the moment, but i would be happy if you combined them since they share this behavior you are adding (and it's by far the most complicated thing about them, so it's probably best to have that doc'd in a single place).

for the tests, all our tests currently use the filesystem by accessing code in the tests/fixtures directory. it's not the best but it's fine for now. mocking is such a pain in rust anyways so i think you are right that this is the least messy way :)

lemme know if you have any other questions.

ashleygwilliams

so i'm pretty sure this is failing because it needs a rebase to include #214. this is approved, i'm goona do the rebase and hopefully get this in today! THANK YOU SO MUCH!!

Return custom error if pkg directory is not found

840c7bd

Mackiovello commented Jul 4, 2018

View reviewed changes

Mackiovello added 3 commits July 5, 2018 18:58

Simplify match expression

035bcbe

Use custom error message for publish

ec17b8a

Make error more specific

8efabfa

Mackiovello added 5 commits July 7, 2018 15:14

Search for path in entered directory and the parent of entered

b57d68f

This lets us have backwards compatibility while improving the user experience.

Use 'find_pkg_directory' for pack

a67762a

Check child directories instead of parent directory

63c91ca

Write as more ideomatic rust

ac34e7b

Simplify iteration even more

e6cd7bb

ashleygwilliams added the work in progress do not merge! label Jul 10, 2018

Fix npm-pack command to use correct path

ce77e5c

ashleygwilliams added this to the 0.5.0 milestone Jul 13, 2018

Merge branch 'master' of https://github.com/rustwasm/wasm-pack into i…

28d5bb5

…ssue/189

Mackiovello changed the title ~~WIP: Improve UX for pack and publish path expectations~~ Improve UX for pack and publish path expectations Jul 14, 2018

ashleygwilliams added needs tests please add tests to this PR needs docs please add docs to this PR and removed work in progress do not merge! labels Jul 16, 2018

ashleygwilliams requested changes Jul 16, 2018

View reviewed changes

Add documentation for pack and publish

88fdbc5

Mackiovello force-pushed the issue/189 branch 3 times, most recently from 90c3fec to 88fdbc5 Compare July 16, 2018 19:23

ashleygwilliams reviewed Jul 19, 2018

View reviewed changes

ashleygwilliams approved these changes Jul 19, 2018

View reviewed changes

ashleygwilliams merged commit e601e70 into rustwasm:master Jul 19, 2018

Mackiovello deleted the issue/189 branch July 19, 2018 16:01

mgattozzi mentioned this pull request Jul 24, 2018

publish and pack commands have confusing path expectations #189

Closed

ashleygwilliams modified the milestones: 0.5.0, 0.4.2 Jul 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve UX for pack and publish path expectations #198

Improve UX for pack and publish path expectations #198

Mackiovello commented Jul 4, 2018 •

edited

Loading

Mackiovello Jul 4, 2018

Mackiovello Jul 4, 2018

Mackiovello Jul 4, 2018

Mackiovello Jul 4, 2018

Mackiovello Jul 4, 2018

Mackiovello commented Jul 5, 2018

ashleygwilliams commented Jul 10, 2018

Mackiovello commented Jul 14, 2018

Mackiovello commented Jul 14, 2018

Mackiovello commented Jul 14, 2018

Mackiovello commented Jul 14, 2018

ashleygwilliams commented Jul 16, 2018

ashleygwilliams left a comment

Mackiovello commented Jul 16, 2018

ashleygwilliams commented Jul 16, 2018

ashleygwilliams left a comment

Improve UX for pack and publish path expectations #198

Improve UX for pack and publish path expectations #198

Conversation

Mackiovello commented Jul 4, 2018 • edited Loading

Mackiovello Jul 4, 2018

Choose a reason for hiding this comment

Mackiovello Jul 4, 2018

Choose a reason for hiding this comment

Mackiovello Jul 4, 2018

Choose a reason for hiding this comment

Mackiovello Jul 4, 2018

Choose a reason for hiding this comment

Mackiovello Jul 4, 2018

Choose a reason for hiding this comment

Mackiovello commented Jul 5, 2018

ashleygwilliams commented Jul 10, 2018

Mackiovello commented Jul 14, 2018

Mackiovello commented Jul 14, 2018

Mackiovello commented Jul 14, 2018

Mackiovello commented Jul 14, 2018

ashleygwilliams commented Jul 16, 2018

ashleygwilliams left a comment

Choose a reason for hiding this comment

Mackiovello commented Jul 16, 2018

ashleygwilliams commented Jul 16, 2018

ashleygwilliams left a comment

Choose a reason for hiding this comment

Mackiovello commented Jul 4, 2018 •

edited

Loading