reading the cargo toml 2x is kinda gross

[ashleygwilliams]

This issue is reserved for someone who has not yet contributed to this codebase.

Currently we read the Cargo.toml twice! this is not preferred:

1. read it to write package.json
2. read it to pass crate name to wasm-bindgen

MENTORING INSTRUCTIONS (from @mgattozzi !):

1. Create a small expression or function to read the Cargo.toml file to a String and assign it to a variable, maybe something like CARGO_TOML
2. Replace instances of it being read with the variable you just created
3. Remove any code associated with reading the file

[mgattozzi]

You could read it in via lazy_static and reference it when you need it rather than reading it twice.

[mgattozzi]

Once #66 lands this will already have lazy_static installed so the steps for that would then be:

1. Create a small expression or function to read the Cargo.toml file to a String and assign it to a variable, maybe something like CARGO_TOML
2. Replace instances of it being read with the variable you just created
3. Remove any code associated with reading the file

It's a small issue, but it'll let you get acquainted with the code base and make things more efficient! :D

[ashleygwilliams]

yesss

[mgattozzi]

#66 landed so this is all good to hack on now :D

[data-pup]

Hi! I'd love to help out with this issue. I can start working on this today, and follow up if I have any further questions 😄

[data-pup]

I had a question about lazy_static. When Cargo.toml file is read, the path variable is used to form the file path. I looked through the docs for the lazy_static macro here, but I am not sure how to declare this as a static variable if the logic to read Cargo.toml requires a parameter.

I see how this could be done by reading the Cargo.toml once in the main function, but wanted to ask if I might be missing something. (Reading the manifest in main seemed like it might be clunky)

[iAmao]

The path is also not constant. I'm a neewb so I'm unsure of how to access it in the manifest file without using functions.

[data-pup]

I took a swing at this! I wasn't able to figure out how to use lazy_static for this, so I moved the read_cargo_toml call up into the main function. The changes I made on my fork are here, should I make a pull request? I can also try again if this doesn't seem like the correct route.

data-pup@73e5ef9

[mgattozzi]

@data-pup I had a whole answer then realized what you meant. I tested this out quickly so I know I won't be sending you down the wrong path. I'll just let you do it and learn! :D

So the big problem here is that there is a lib.rs and a main.rs file. You can't access the manifest file and the path handed to the program without knowing the arguments. They actually shouldn't be separate for a commandline project like this. Here's what you'll need to do to get this working:

1. Put everything in lib.rs into main.rs instead and remove lib.rs
2. Clean up the imports and remove the extern crate wasm_pack in main.rs
3. At this point everything should compile and run just fine. Run it here just to be sure. When it compiles and runs fine then you can proceed to four
4. Now you need to get the path which means you need to parse the args! The main!() macro from quicli is actually hiding this, but we too need these! The good thing is you can parse args in more than one place!

So in the block for it you'll need something like this:

  let x = Cli::from_args();
  match x.cmd {
         Command::Init { path, scope } => {
             // Read the file here with the path and return the string
         },
         // Match the other ones here, see below in main.rs for help on how too
   }

Path is an Option<String>. If you have a Some(path) then use that path otherwise default to using . (current directory) in the case of None.

Then proceed with the other instructions.

Thanks for pointing this out I didn't realize this was going to be an issue when making the instructions. This should do it based off the quick protoype I whipped up but it was more just checking that you could do the args in lazy_static so no file reading or anything.

Try this out and if it works send a PR in and we can do a review and help you polish and clean any rough edges! :D If this isn't detailed enough or you need more clarity please let me know :D

[mgattozzi]

@data-pup I also just saw your comment after typing this out. That also works as a solution, but we're adding more argument calls to functions, which we can avoid here with lazy-static. It'll help keep our signature functions less "noisy" and allow us to easily use that data in other places later on. Try the above out and if that's still not possible at all then lets go this route, but I'm certain we can get it working with lazy-static :)

[ashleygwilliams]

hey friends, i'm gonna put a hold on this issue. i don't want to get rid of lib... it's literally the entire strategy behind how this application is tested so we can't get rid of it! gimme a sec to think and i'll let ya know.

[data-pup]

@mgattozzi Thanks for following up, that makes sense to me! Definitely agree about the fact that adding arguments seemed kind of noisy. I'll hold up on this for now, per Ashley's comment above, but I would love to help out once a strategy is decided on :)

[ashleygwilliams]

thanks so much for your efforts and patience @data-pup ! i'll def ping u once we figure out what we want here, i think i'm bumping into a frustration have with testing rust binaries ;) we'll see how i sort that

[data-pup]

Of course! Sounds good to me : ) Good luck!

[mgattozzi]

In the meantime if there's another issue you want to take a crack at definitely do so! :D

[ashleygwilliams]

oh yeah plz @data-pup - even if it isn't marked as helpwanted or good first issue, lemme know and we'll get u set up with it. i feel bad about this sitch :( <3 and want u to get a chance to contribute!

[bringking]

@ashleygwilliams is this something that is ready for work? I would love to try and tackle this

[ashleygwilliams]

@bringking i believe after several refactors we should be able to tackle this- that being said @data-pup had originally tried to tackle this, so i'd like to give them right of first refusal before someone else takes it on- what do you think @data-pup ?

[data-pup]

Hey! I would love to take another swing at this :)

[bringking]

@ashleygwilliams okay cool, works for me 😃

[mgattozzi]

@bringking There are definitely other issues open to work on! If you see one that catches your interest say you are and we can write out instructions on whatever needs to be done to get it working if you need them :D

[data-pup]

@mgattozzi I tried to add a static CARGO_TOML variable using the env::args method we discussed, but it ended up causing tests to fail. The initial changes I made are here. I can open a PR for that branch if you'd like, but this issue might be worth leaving alone for the time being?

CargoManifest.into_npm(..), used in write_package_json borrows self mutably. We could get around that with an NpmPackage::from(manifest) trait or something, but that would mean more refactoring.
Most troublesome however, is that (as far as I can tell?) we would need to rethink how the test suite works, if the Cargo.toml file is read using the program arguments.

Let me know if you have any advice, otherwise I think I'll try and work on some other issues in the meantime :)

[mgattozzi]

Hmmm so cloning is basically happening which is essentially reading it twice.

Since we're just serializing data which only requires a reference we could get npm to just hold &str instead of String. Rather than it taking ownership it borrows so this should work!

Also in the case of it not failing when initializing, have it be a Result and just use ? in both places it's called!

[data-pup]

Ah neat! That makes sense, I’ll try that out tonight 🙂

[data-pup]

Turning the static variable into a Result<CargoManifest, Error> object did not seem to work for me. I ended up running into errors like this:

error[E0277]: the `?` operator can only be applied to values that implement `std::ops::Try`
   --> src/manifest.rs:146:20
    |
146 |     let manifest = CARGO_TOML?; // CAUSES ERROR
    |                    ^^^^^^^^^^^ the `?` operator cannot be applied to type `manifest::CARGO_TOML`
    |
    = help: the trait `std::ops::Try` is not implemented for `manifest::CARGO_TOML`
    = note: required by `std::ops::Try::into_result`

I found some advice online elsewhere saying something like &*CARGO_TOML would be a way around this, but that did not seem to help either. The closest I could get to fixing that was using .as_ref(), but that meant the try operator wasn't working, because I had a reference to an error, rather than an error.

The other problem I was seeing is that this approach would break the get_crate_name test cases, because the path parameter is not used by the static variable :(

[mgattozzi]

Ah okay. I had a very busy weekend so I'm just getting around to this. Do you have the code up somewhere? I can take a look and see if there's a way around the weirdness. There's a lot of things lazy_static does to do what it does and occasionally you run into problems like this.

[data-pup]

No worries! I have a branch on my fork of this repo with my work on this here.

I was noticing that about lazy_static, it’s actually a really neat macro! Troubles aside, glad I’ve been able to learn more about it bc of this 🙂

[mgattozzi]

@data-pup it's a real neat macro and one of my favorites. I'll take a look at this tonight.