Don't install wasm-bindgen if it already exists #79
Conversation
…eck if 'wasm-bindgen' already exists in the path. This might need some revisions to be Windows compatible.
src/bindgen.rs
Outdated
|
|
||
| fn wasm_bindgen_installed() -> bool { | ||
| if let Ok(path) = env::var("PATH") { | ||
| return path.split(":") |
There was a problem hiding this comment.
Since PATH is just a String you can test by doing path.contains("wasm-bindgen")
There was a problem hiding this comment.
You also don't need to use return just leave off the semicolon! :D
Rust will always return a value for the last expression without a semicolon. We use return when we want to exit a function early much like you did in the file above!
There was a problem hiding this comment.
Ah, thanks! I might have misunderstood, but is wasm-bindgen added to PATH, or would that be an item within one of PATH's directories?
Regarding returns, I will make these changes. Nice to know about that convention!
There was a problem hiding this comment.
Oh you know what you're right. PATH contains directories of binaries that can be run not the binaries themselves! You'll need to traverse them to find it. I'd like to say we can assume it's in .cargo/bin and just check there, which is easier. However, I feel we should probably check each one like you've done already.
Regarding cross platform you can do conditional compilation depending on the platform so we can set a constant in the file and use it here to say "on this platform use : and on this one use" whatever it is.
src/bindgen.rs
Outdated
| }) | ||
| .fold(false, |res, b| res || b); | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
Same thing here you can just put false!
| @@ -36,3 +37,16 @@ pub fn wasm_bindgen_build(path: &str, name: &str) { | |||
| .unwrap_or_else(|e| panic!("{} failed to execute process: {}", emoji::ERROR, e)); | |||
| pb.finish(); | |||
| } | |||
|
|
|||
| fn wasm_bindgen_installed() -> bool { | |||
| if let Ok(path) = env::var("PATH") { | |||
There was a problem hiding this comment.
Normally I'd say we should handle error cases but if your PATH variable isn't set I think you have bigger problems so I think this is fine for now. I plan on going through and cleaning up errors in general later on and I think that is outside the scope of this PR
|
Also your Travis CI build failed because of the need to run rust-fmt. Do you know how to do that or do you have installed? If you don't I can write up some instructions for you |
|
Awesome, thank you for the help! Regarding the |
|
awesome! you should be able to add it with rustup and then you'll be able to just do cargo fmt and it'll do it all for you! |
|
These changes look great @data-pup! I believe this should work just from reading the code, but it would be good to confirm. I have a windows and linux build I can try these on at some point. Also you'll need to format the changes but I believe this should be good. @ashleygwilliams tagging you in case you haven't seen this yet |
|
I added some edits in a second commit containing some of the changes discussed above. I installed and ran rust-fmt, but it seems like the Travis build is still failing? I am not sure what to do about that 😕 |
|
You'll need to run it and commit the changes for it. Again. My guess is it didn't run on everything somehow. |
I started working on some changes to try and fix the problems mentioned in #51. Per the discussion on that issue, I am opening up a PR so that the changes made can be reviewed and discussed. I don't think this is ready to merge quite yet, but I have some questions about testing, cross-platform issues (specifically Windows), and general style.
This change adds a function called
wasm_bindgen_installed, which will return a boolean value representing whether or notwasm-bindgenexists in the path. Thecargo_install_wasm_bindgenwill only progress to the installation process ifwasm_bindgen_installedreturns false.Questions:
Testing
I am not exactly sure how to design test cases for this, since it depends on the environment. I did test this manually and things seemed to be working correctly, but ideally some test cases would make sure that this behaves correctly.
Windows
Windows uses
;as a path separator, in contrast to the:separator on Linux. Should I add a separator variable using thecfg!macro to set this accordingly? Are there other compatibility issues to consider aside from that?Style
Ashley sent me a link to this SO answer regarding how to check if a command is in the path, which I used as a starting point for this. The code below uses
mapandfoldinstead of a for loop as shown in the SO answer. Is there a preference regarding that? I do not mind rewriting this to follow an imperative style.P.S. Thank you for being welcoming to contributions! I am still a little new to helping out with open source projects, so I really appreciate it. :)
Update: The Travis build did not pass, this seems to have failed:
The command "cargo fmt -- --write-mode diff" exited with 4.Is that a formatting issue?