Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

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

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

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

Conversation

@vramana

@vramana vramana commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

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

anonrig commented Jul 18, 2023

Copy link
Copy Markdown
Member

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

vramana commented Jul 18, 2023

Copy link
Copy Markdown
Contributor Author

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

Sure

@anonrig

anonrig commented Jul 18, 2023

Copy link
Copy Markdown
Member

Is this PR ready to get merged?

@vramana

vramana commented Jul 18, 2023

Copy link
Copy Markdown
Contributor Author

@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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants