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

feat: Add cargo component add command #18

Merged
merged 7 commits into from
Apr 27, 2022
Merged

feat: Add cargo component add command #18

merged 7 commits into from
Apr 27, 2022

Conversation

saulecabrera
Copy link
Member

@saulecabrera saulecabrera commented Apr 25, 2022

Implements #5

This commit introduces the add command, analogous to cargo add for
adding dependendcies to the manifest.

This command validates:

  1. Name conflicts between dependencies
  2. Only 1 default export can exist

For reference, this the help output of the subcommand:

cargo-component-add 0.1.0
Add a depency for a WebAssembly component

USAGE:
    cargo component add [OPTIONS] --path <PATH> <name>

ARGS:
    <name>    Name of the dependency

OPTIONS:
        --color <WHEN>            Coloring: auto, always, never
        --dry-run                 Don't actually write the manifest
        --export                  Set the dependency as an exported interface
    -h, --help                    Print help information
        --manifest-path <PATH>    Path to Cargo.toml
    -p, --package <SPEC>          Package to add the dependency to (see `cargo help pkgid`)
        --path <PATH>             Path to the interface definition of the dependency
    -q, --quiet                   Do not print cargo log messages
    -v, --verbose                 Use verbose output (-vv very verbose/build.rs output)
        --version <VERSION>       Set the version of the dependency


Some open questions around the design:

  1. The command is currently not validating the existence/validity of the .wit file; I left it this way operating under the assumption that it's a concern of the build command to validate the existence of the interface file. Also, I can see a possibility of this command being used as a starting point to get the Cargo.toml changes ready, rather than as a final step. That said, if you think this should be validated at this step, I can definitely add that in.
  2. The --dry-run option currently prints the un-written state of the manifest to the stdout, not sure if there's a better course of action here.

This commit introduces the `add` command, analogous to `cargo add` for
adding dependendcies to the manifest.

This command validates:

1. Name conflicts between dependencies
2. Only 1 default export can exist
Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

This looks great to me, just a few comments.

Regarding the validation of the interface file, I think we might as well check it at least exists when adding it to Cargo.toml; as to whether or not it parses as an interface definition, I'm fine with leaving that up to the other commands that need to read the file contents.

src/commands/add.rs Outdated Show resolved Hide resolved
src/commands/add.rs Outdated Show resolved Hide resolved
src/commands/add.rs Outdated Show resolved Hide resolved
This commit simplifies the package option to make it singular, this
makes it easier to validate edge cases around multiple package
specifications
Add a more specific and standard message for the recently added
dependency
Use `find` instead of `map` + `contains`
@saulecabrera
Copy link
Member Author

@peterhuene thanks for the review, this should be ready for another pass. I also realized that in my original implementation I was not validating the presence of the version if the dependency is an import. I added that in 35481c2

Copy link
Member

@peterhuene peterhuene left a comment

Choose a reason for hiding this comment

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

👍

@peterhuene peterhuene merged commit bf8d0be into main Apr 27, 2022
@peterhuene peterhuene deleted the add-command branch April 27, 2022 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants