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.

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

generate #623

Merged
merged 3 commits into from
Jun 11, 2019
Merged

generate #623

merged 3 commits into from
Jun 11, 2019

Conversation

ashleygwilliams
Copy link
Member

@ashleygwilliams ashleygwilliams commented Apr 11, 2019

fixes #373

  • todo: respect noinstall
  • todo: lookup version of latest
  • docs
  • tests

@ashleygwilliams ashleygwilliams added this to the 0.9.0 milestone Apr 11, 2019
@ashleygwilliams ashleygwilliams added needs docs please add docs to this PR needs tests please add tests to this PR labels Apr 11, 2019
@ashleygwilliams
Copy link
Member Author

this is still a WIP since i need to sort the testing but i'd love some feedback on the approach. additionally @alexcrichton if you rebase your #625 i think you can take advantage of some of the refactoring i've done here

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

All's looking good to me!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
The `wasm-pack generate` command can be given and optional name argument, e.g.:

```
wasm-pack generate --name myproject
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it might be better to wasm-pack generate myproject? That jives with cargo new and I think commands like git init as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

so the reason i did a flag was that you can also pass --template.. we could have name be positional tho and still have the other flags. i think conforming to similar tools makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah I meant just having --name be a positional argument instead (but keeping --template). We could perhaps take a leaf out of git clone's book though and do something like:

# generates a new project named "foo"
$ wasm-pack generate https://github.com/alexcrichton/foo

# generates a new project named "bar"
$ wasm-pack generate https://github.com/alexcrichton/foo bar

I don't really feel too strongly any direction, but I do think that having the name be a positional argument rather than a flag might be nice.

Copy link
Member Author

Choose a reason for hiding this comment

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

i really like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think this would be a neat feature but let's put it off til another PR as i'm worried this one is getting a lil big

src/generate.rs Outdated Show resolved Hide resolved
use serde::Deserialize;

#[derive(Debug, Deserialize)]
pub struct Krate {
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I typically only use "krate" for the literal name "crate", but other variations like "Crate" I tend to use instead of "Krate" b/c it's valid Rust both ways (only crate is invalid)

src/install/mod.rs Outdated Show resolved Hide resolved
@@ -23,6 +23,7 @@ glob = "0.2"
log = "0.4.6"
openssl = { version = '0.10.11', optional = true }
parking_lot = "0.6"
reqwest = "0.9.14"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this perhaps use curl to use the same downloading backend as binary-install?

Copy link
Member Author

Choose a reason for hiding this comment

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

would you mind if we did this in another PR- it's... a little tricky with the interface and i'm worried this PR is getting pretty big cc @drager

Copy link
Member

Choose a reason for hiding this comment

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

Personally I think that would be fine, as long as we have a tracking issue for it and planning to do it in the near future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, deferring it to an issue seems fine

@ashleygwilliams ashleygwilliams removed the needs docs please add docs to this PR label Apr 12, 2019
@alexcrichton
Copy link
Contributor

To bikeshed a little bit, if we ignore backwards compatibility and that wasm-pack init was historically a subcommand, would be perhaps best named as wasm-pack init or wasm-pack new? This seems akin to cargo new and cargo init to me (which arguably both should take template parameters)

@ashleygwilliams
Copy link
Member Author

@alexcrichton i honestly had the same feeling when i was making this PR, new and init seem better and are a lot easier to type.

i think init has been deprecated for a lot of cycles at this point. i'd be open to replacing it, or we can go with new.

@ashleygwilliams ashleygwilliams removed the needs tests please add tests to this PR label Apr 12, 2019
@alexcrichton
Copy link
Contributor

I think I'd vote for new over init since it's creating directories rather than initializing pre-existing ones (sorta like cargo new/init), but I'm not sure how much precedent there is for that distinction honestly

src/command/mod.rs Outdated Show resolved Hide resolved
src/generate.rs Outdated Show resolved Hide resolved
@ashleygwilliams ashleygwilliams removed the work in progress do not merge! label Apr 15, 2019
@ashleygwilliams ashleygwilliams changed the title [WIP] generate generate Apr 15, 2019
@alexcrichton
Copy link
Contributor

One recommendation I might have for testing comes from Cargo's test suite which is to use local file paths to git repositories. The general idea would be to make a builder-style API which creates a bunch of files in a directory, and then has a build() method which writes it all out and then turns it into a git repository with one commit as well. The result of build() could then be a Url which is actually just a file path, and then wasm-pack generate is invoked with that URL to ensure that it basically calls cargo generate and the right results show up (pass in a few different names, etc)

The various git helpers for Cargo are located in https://github.com/rust-lang/cargo/blob/master/tests/testsuite/support/git.rs and largely rely on git2, but using the CLI here would probably also be fine.

@@ -3,9 +3,14 @@
`wasm-pack` has several commands to help you during the process of building
a Rust-generated WebAssembly project.

- `init`: This command has been deprecated in favor of `build`.
- `new`: This command generates a new project for you using a template. [Learn more][generate]
Copy link
Member

Choose a reason for hiding this comment

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

Broken link here? Shouldn't it be [Learn more][new]?

```

The mode pass can be either "normal", "noinstall", or "force". "normal is passed by
degault.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
degault.
default.

@@ -52,8 +52,8 @@ matrix:
- cargo fmt --version
- cargo fmt --all -- --check
- rustup component add clippy-preview
- cargo clippy --version
- cargo clippy
# - cargo clippy --version
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be brought back?

1. `cd hello-wasm`
1. Run `wasm-pack build`.
1. This tool generates files in a `pkg` dir
1. To publish to npm, run `wasm-pack publish`. You may need to login to the
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider mention that publishing to npm is optional?

@drager
Copy link
Member

drager commented May 16, 2019

@ashleygwilliams About the tests I agree with you that some more test would be nice to have. However, I think we can add those afterwards. Let's get this PR merged first.

@ashleygwilliams ashleygwilliams merged commit a267075 into master Jun 11, 2019
@ashleygwilliams ashleygwilliams deleted the generate branch June 11, 2019 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundle cargo generate temporarily for ease of installation
3 participants