Skip to content

feat: implement -D, -O options for add command#12

Merged
anonrig merged 1 commit intopnpm:mainfrom
vramana:add-commands
Jul 18, 2023
Merged

feat: implement -D, -O options for add command#12
anonrig merged 1 commit intopnpm:mainfrom
vramana:add-commands

Conversation

@vramana
Copy link
Copy Markdown
Contributor

@vramana vramana commented Jul 18, 2023

One caveat is if a package already exists in another dependecy group, we don't remove the existing entry. Can we fix it in another PR?

Comment thread crates/package_json/src/lib.rs Outdated
value: Value,
}

pub enum DependencyType {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should this be called DependencyGroup instead? Should I add peer dependencies, bundled dependencies in this PR?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like dependencygroup better, but I'm ok with dependencytype as well. I think we can split the PRs. That's ok

Comment thread crates/cli/src/commands.rs Outdated
} else if self.optional {
DependencyType::Optional
} else {
DependencyType::Normal
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you think about naming this as Default?

Comment thread crates/package_json/src/lib.rs Outdated
value: Value,
}

pub enum DependencyType {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like dependencygroup better, but I'm ok with dependencytype as well. I think we can split the PRs. That's ok

Comment thread crates/package_json/src/lib.rs Outdated
Optional,
}

impl Into<&str> for DependencyType {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you also add From trait as well?

Comment thread crates/cli/src/commands.rs Outdated
#[arg(short = 'D', group = "dependency_type")]
dev: bool,
/// Add the package as a optional dependency
#[arg(short = 'O', group = "dependency_type")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

long value should be added as save-optional

Comment thread crates/cli/src/commands.rs Outdated
/// Name of the package
pub package: String,
/// Add the package as a dev dependency
#[arg(short = 'D', group = "dependency_type")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

long value should be added as save-dev

@anonrig
Copy link
Copy Markdown
Member

anonrig commented Jul 18, 2023

One caveat is if a package already exists in another dependecy group, we don't remove the existing entry. Can we fix it in another PR?

Can you add a TODO in the code so we don't forget?

@vramana
Copy link
Copy Markdown
Contributor Author

vramana commented Jul 18, 2023

Can you add a TODO in the code so we don't forget?

Sure

@anonrig
Copy link
Copy Markdown
Member

anonrig commented Jul 18, 2023

Is this PR ready to get merged?

@vramana
Copy link
Copy Markdown
Contributor Author

vramana commented Jul 18, 2023

@anonrig Yes!

@anonrig anonrig merged commit fe613e2 into pnpm:main Jul 18, 2023
@vramana vramana deleted the add-commands branch July 18, 2023 16:56
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.

2 participants