feat(cli): add deno bump-version subcommand
#30562
Conversation
Signed-off-by: John Carveth <[email protected]>
Signed-off-by: John Carveth <[email protected]>
Signed-off-by: John Carveth <[email protected]>
Signed-off-by: John Carveth <[email protected]>
Signed-off-by: John Carveth <[email protected]>
|
Hey @JLCarveth we'll try to give you initial review this week. Is there anything that is blocking you at the moment? |
Should I look into setting a default value for the |
|
@JLCarveth I'm not sure - |
|
@JLCarveth we just discussed this during the CLI meeting - overall this looks good, but we have a couple comments:
|
|
The Git integration was based on the If the team would prefer a simpler integration it could be removed |
|
After thinking on it further, I agree that the git integration is not necessary. Adds more maintenance burden when a simple bash function can handle the git stuff. I'll remove that tomorrow |
|
@JLCarveth sounds good, can you also rename the subcommand to |
Signed-off-by: John Carveth <[email protected]>
Signed-off-by: John Carveth <[email protected]>
Signed-off-by: John Carveth <[email protected]>
|
@JLCarveth I think we're ready to land this one - I'm thinking we could do it this week, without waiting for another minor release - how about we land this one as "unstable"? If so, let's add a small banner anytime this command is run that says: |
cli/args/flags.rs
Outdated
| <p(245)>deno bump-version patch --no-git-tag</> # Don't create git tag | ||
| <p(245)>deno bump-version patch --git-commit-all</> # Stage and commit all changes" |
There was a problem hiding this comment.
Remove? Doesn't seem to be implemented (same with --dry-run)
cli/tools/bump_version.rs
Outdated
| if let Some(deno_json) = start_dir.maybe_deno_json() { | ||
| let config_path = deno_path_util::url_to_file_path(&deno_json.specifier) | ||
| .context("Failed to convert deno.json URL to path")?; | ||
| configs.push(ConfigUpdater::new(ConfigKind::DenoJson, config_path)?); |
There was a problem hiding this comment.
Is this ConfigKind necessary? Seems we could remove it?
cli/tools/bump_version.rs
Outdated
| return Ok(()); | ||
| } | ||
| // Default to 1.0.0 if no version is found but increment is specified | ||
| Version::parse_standard("1.0.0") |
There was a problem hiding this comment.
Maybe it should default to 0.1.0? If someone accidentally publishes a 0.1.0 version it's not too bad, but a 1.0.0 version can be bad. Plus, if someone has never published a version then most likely it's currently experimental, so 0.1.0 might make more sense.
Signed-off-by: John Carveth <[email protected]>
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Looks good from me. @dsherret are you okay with landing this as is?
cli/tools/bump_version.rs
Outdated
| // Check for package.json | ||
| if let Some(pkg_json) = start_dir.maybe_pkg_json() { | ||
| configs.push(ConfigUpdater::new(pkg_json.path.clone())?); | ||
| } |
There was a problem hiding this comment.
I'm not sure about this updating both deno.json and package.json at the same time.
I think probably this sub command should only update deno.json and then fallback to package.json when the deno.json doesn't exist (maybe also skipping it when the deno.json has no "name" and "version" field). Then in the future we can add an explicit flag for only updating the package.json (ex. deno bump-version --package-json minor or deno bump-version --npm minor)
Thoughts?
There was a problem hiding this comment.
I actually agree - we should do one or the other, not both at the same time. Defaulting to deno.json is fine, using package.json when no deno.json is fine and adding --package-json to force changing package.json is fine too for me.
dsherret
changed the title
deno bump-version subcommand
Signed-off-by: John Carveth <[email protected]>
Signed-off-by: John Carveth <[email protected]>
WalkthroughAdds a new Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/integration/mod.rs (1)
49-50: Module placement breaks alphabetical order.The other modules are alphabetically sorted.
bump_versionshould be placed afterbench(line 11) rather than afterpublish.#[path = "bench_tests.rs"] mod bench; +#[path = "bump_version_tests.rs"] +mod bump_version; #[path = "cache_tests.rs"] mod cache;And remove lines 49-50.
cli/tools/bump_version.rs (1)
74-78: Consider insertion position for new version field.Inserting at index 0 places
versionbeforename, which is unconventional. Typicallynamecomes first in package manifests. Consider inserting afternameif it exists, or at index 0 otherwise.This is a minor stylistic concern - the current behavior is functional.
cli/args/flags.rs (1)
4137-4164: Clarify semantics of optionalincrementand de-duplicate parsing logicRight now:
- The positional
incrementis optional (norequired_*), sodeno bump-versioncan be invoked without an argument andVersionFlags { increment: None }is produced.- Valid strings are enforced twice: once in
value_parser([...])and again in the manual match inbump_version_parse.If the design is that an increment is required (like
npm version), consider making it explicit:- cmd.arg( - Arg::new("increment") - .help("Version increment type") + cmd.arg( + Arg::new("increment") + .help("Version increment type") .value_parser([ "major", "minor", "patch", "premajor", "preminor", "prepatch", "prerelease", ]) - .index(1), + .index(1) + .required_unless_present("help"), )And to avoid the parser and
bump_version_parseever drifting, you can centralize the mapping:-fn bump_version_parse(flags: &mut Flags, matches: &mut ArgMatches) { - let increment = - matches - .remove_one::<String>("increment") - .and_then(|s| match s.as_str() { - "major" => Some(VersionIncrement::Major), - "minor" => Some(VersionIncrement::Minor), - "patch" => Some(VersionIncrement::Patch), - "premajor" => Some(VersionIncrement::Premajor), - "preminor" => Some(VersionIncrement::Preminor), - "prepatch" => Some(VersionIncrement::Prepatch), - "prerelease" => Some(VersionIncrement::Prerelease), - _ => None, - }); +fn parse_version_increment(s: &str) -> VersionIncrement { + match s { + "major" => VersionIncrement::Major, + "minor" => VersionIncrement::Minor, + "patch" => VersionIncrement::Patch, + "premajor" => VersionIncrement::Premajor, + "preminor" => VersionIncrement::Preminor, + "prepatch" => VersionIncrement::Prepatch, + "prerelease" => VersionIncrement::Prerelease, + _ => unreachable!(), + } +} + +fn bump_version_parse(flags: &mut Flags, matches: &mut ArgMatches) { + let increment = matches + .remove_one::<String>("increment") + .map(|s| parse_version_increment(&s)); @@ flags.subcommand = DenoSubcommand::BumpVersion(VersionFlags { increment }); }Finally, it’d be good to add a small
flags_from_vectest fordeno bump-version(with and withoutincrement) alongside the other CLI parsing tests, so the new wiring doesn’t regress silently.Also applies to: 5442-5458
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cli/args/flags.rs(7 hunks)cli/main.rs(1 hunks)cli/tools/bump_version.rs(1 hunks)cli/tools/mod.rs(1 hunks)tests/integration/bump_version_tests.rs(1 hunks)tests/integration/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
cli/tools/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI tools should be implemented in
cli/tools/<tool>orcli/tools/<tool>/mod.rs
Files:
cli/tools/mod.rscli/tools/bump_version.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
cli/tools/mod.rstests/integration/mod.rscli/tools/bump_version.rstests/integration/bump_version_tests.rscli/main.rscli/args/flags.rs
cli/main.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Main CLI entry point is in
cli/main.rsand should handle command routing
Files:
cli/main.rs
cli/args/flags.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI flag parsing should be defined in
cli/args/flags.rs
Files:
cli/args/flags.rs
🧠 Learnings (4)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/tools/**/*.rs : CLI tools should be implemented in `cli/tools/<tool>` or `cli/tools/<tool>/mod.rs`
Applied to files:
cli/tools/mod.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/module_loader.rs : Module loading and resolution is handled in `cli/module_loader.rs`
Applied to files:
cli/tools/mod.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to src/**/*.rs : Unit tests should be inline with source code in each module
Applied to files:
tests/integration/mod.rs
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/args/flags.rs : CLI flag parsing should be defined in `cli/args/flags.rs`
Applied to files:
cli/args/flags.rs
🧬 Code graph analysis (2)
tests/integration/bump_version_tests.rs (2)
cli/tools/bump_version.rs (1)
new(30-46)cli/main.rs (4)
output(80-80)output(84-86)output(90-92)output(96-98)
cli/main.rs (1)
cli/tools/bump_version.rs (1)
bump_version_command(191-240)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: lint debug macos-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (8)
cli/tools/mod.rs (1)
4-4: LGTM!Module placement follows the alphabetical convention. Based on learnings, CLI tools should be in
cli/tools/<tool>- this follows the expected pattern.cli/main.rs (1)
442-448: LGTM!The experimental warning follows the same pattern as
deno bundle. The subcommand is correctly wired to the bump_version_command.tests/integration/bump_version_tests.rs (1)
1-229: Solid test coverage.Tests cover the key scenarios: increment types (patch/minor/major/prerelease), config file types (deno.json/package.json), edge cases (missing config, missing version field, read-only query). Structure preservation is also validated.
cli/tools/bump_version.rs (3)
95-171: Increment logic is correct.The implementation follows standard semver incrementing rules. Pre-release identifiers are properly handled for all increment types.
One note: the function always succeeds, so the
Result<Version, AnyError>return type could be simplified to justVersion. However, keepingResultallows for future error cases without API changes.
173-189: Config file resolution follows the agreed approach.Correctly prioritizes deno.json over package.json, with clear error messaging when neither exists. This aligns with the past review discussion.
191-240: Command implementation is clean.The flow is logical: find config → get/default version → increment or display → commit. Default version is 0.1.0 as requested in past reviews.
cli/args/flags.rs (2)
136-151: VersionFlags / VersionIncrement and DenoSubcommand wiring look soundThe new
VersionFlags/VersionIncrementtypes and theDenoSubcommand::BumpVersion(VersionFlags)variant follow the existing patterns for other subcommands (simple flag struct + enum variant) and don’t interfere withwatch_flags,is_run, orneeds_test. No issues here.Also applies to: 552-589
1527-1535: bump-version is correctly exposed in help, dispatch, and root clap config
- Help entry under “Dependency management” matches the new subcommand name and intent.
flags_from_vec_with_initial_cwddispatches"bump-version"tobump_version_parseandclap_rootregistersbump_version_subcommand()with the same name, so the CLI routing is consistent end‑to‑end.
No functional problems spotted.Also applies to: 1711-1757, 2013-2019
| <g>uninstall</> Uninstalls a dependency or an executable script in the installation root's bin directory | ||
| <g>outdated</> Find and update outdated dependencies | ||
| <g>remove</> Remove dependencies from the configuration file | ||
| <g>publish</> Publish the current working directory's package or workspace |
There was a problem hiding this comment.
Can you move this back? I don't believe this is dependency management?
There was a problem hiding this comment.
I added this, I think it's fine
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cli/args/flags.rs (3)
1556-1566: Help text could mention prerelease incrementsYou already accept
premajor,preminor,prepatch, andprereleasevia the CLI, but the top‑level help examples only showmajor|minor|patch. Consider adding one prerelease example or a brief mention so users discover those modes.
4226-4253: bump_version_subcommand definition is minimal and clearCommand name, description, and the positional
incrementargument are all wired correctly, and the value parser matches the supported increments. If you want the help output a bit clearer, you could add avalue_name("INCREMENT"), but that’s optional.
5533-5549: bump_version_parse maps CLI to VersionFlags correctly; consider adding unit testsThe parse logic cleanly converts the optional
incrementstring intoOption<VersionIncrement>and setsflags.subcommandappropriately. To keep this file’s test coverage in line with other subcommands, consider adding a small test or two (for no‑arg and e.g.patch) in thetestsmodule to assert the resultingFlags { subcommand: DenoSubcommand::BumpVersion(..) }.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/args/flags.rs(7 hunks)cli/main.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or uselldbdirectly
Useeprintln!()ordbg!()macros for debug prints in Rust code
Files:
cli/main.rscli/args/flags.rs
⚙️ CodeRabbit configuration file
Don't worry about coverage of Rust docstrings. Don't be nitpicky about it. Leave it to the author's judgement if such a documentation is necessary.
Files:
cli/main.rscli/args/flags.rs
cli/main.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Main CLI entry point is in
cli/main.rsand should handle command routing
Files:
cli/main.rs
cli/args/flags.rs
📄 CodeRabbit inference engine (CLAUDE.md)
CLI flag parsing should be defined in
cli/args/flags.rs
Files:
cli/args/flags.rs
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/args/flags.rs : CLI flag parsing should be defined in `cli/args/flags.rs`
Applied to files:
cli/args/flags.rs
🧬 Code graph analysis (1)
cli/main.rs (1)
cli/tools/bump_version.rs (1)
bump_version_command(191-240)
🔇 Additional comments (5)
cli/main.rs (1)
453-459: LGTM!The new
BumpVersionsubcommand handler follows the established patterns in this file. The experimental warning is present as requested, and the function call correctly passes theArc<Flags>andVersionFlagsas expected bybump_version_command.cli/args/flags.rs (4)
138-152: VersionFlags / VersionIncrement definitions look soundThe enum and flags struct cleanly model the supported increments and integrate well with the rest of the flags types; no issues here.
568-605: BumpVersion subcommand variant is correctly integrated into DenoSubcommandAdding
BumpVersion(VersionFlags)here is consistent with existing subcommands and keeps pattern‑matching straightforward.
1752-1788: bump-version dispatch wiring is correctThe new
"bump-version"arm callingbump_version_parsematches the pattern of other non‑fallible parsers likeremove/clean; control flow and error handling look fine.
2015-2051: clap_root registration of bump-version is consistentRegistering
bump_version_subcommand()alongside other dependency‑management subcommands keeps the CLI surface coherent; no issues here.
3 participants
Adds a
deno bump-versionsubcommand similar tonpm versiondeno initshould be updated to include a default value for theversionfield indeno.json.versionfield it finds in eitherdeno.json(c)orpackage.json, but perhaps we would instead prefer to check both files explicitly in case there are discrepancies.Closes #30358