Skip to content

Update move command to accept standard mv args#173

Open
martinemde wants to merge 2 commits intomainfrom
cursor/update-move-command-to-accept-standard-mv-args-composer-1-a8e5
Open

Update move command to accept standard mv args#173
martinemde wants to merge 2 commits intomainfrom
cursor/update-move-command-to-accept-standard-mv-args-composer-1-a8e5

Conversation

@martinemde
Copy link

  • I bumped the gem version (or don't need to) 💎

Aligns the bin/packs move command's argument order with the standard mv utility.

The command now accepts arguments as sources... destination (last argument is the destination pack), making it more intuitive and consistent with common CLI patterns.


Slack Thread

Open in Cursor Open in Web

Co-authored-by: martin.emde <martin.emde@gusto.com>
@cursor
Copy link

cursor bot commented Nov 25, 2025

Cursor Agent can help with this pull request. Just @cursor in comments and I'll start working on changes in this branch.
Learn more about Cursor Agents

@martinemde
Copy link
Author

@cursor Make sure sorbet passes:

lib/packs/cli.rb:99: Expected `String` but found `T.nilable(String)` for argument `pack_name` https://srb.help/7002
    99 |        pack_name: pack_name,
                           ^^^^^^^^^
  Expected `String` for argument `pack_name` of method `Packs.move_to_pack!`:
    sorbet/rbi/gems/packs-specification@0.0.10.rbi:92:
    92 |        pack_name: ::String,
                ^^^^^^^^^
  Got `T.nilable(String)` originating from:
    lib/packs/cli.rb:99:
    99 |        pack_name: pack_name,
                           ^^^^^^^^^
  Autocorrect: Use `-a` to autocorrect
    lib/packs/cli.rb:99: Replace with `T.must(pack_name)`
    99 |        pack_name: pack_name,
                           ^^^^^^^^^

lib/packs/cli.rb:100: Expected `T::Array[String]` but found `T.nilable(T::Array[String])` for argument `paths_relative_to_root` https://srb.help/7002
     100 |        paths_relative_to_root: paths
                                          ^^^^^
  Expected `T::Array[String]` for argument `paths_relative_to_root` of method `Packs.move_to_pack!`:
    sorbet/rbi/gems/packs-specification@0.0.10.rbi:93:
    93 |        paths_relative_to_root: T::Array[::String],
                ^^^^^^^^^^^^^^^^^^^^^^
  Got `T.nilable(T::Array[String])` originating from:
    lib/packs/cli.rb:100:
     100 |        paths_relative_to_root: paths
                                          ^^^^^
  Autocorrect: Use `-a` to autocorrect
    lib/packs/cli.rb:100: Replace with `T.must(paths)`
     100 |        paths_relative_to_root: paths
                                          ^^^^^
Errors: 2

@cursor
Copy link

cursor bot commented Nov 25, 2025

Fixed the Sorbet type errors by adding T.must() to assert non-nil values after the length check. Since we validate args.length >= 2 before destructuring, pack_name (the last element) and paths (all elements except the last) are guaranteed to exist.

The changes:

  • Added T.must(pack_name) to ensure Sorbet knows pack_name is a String, not T.nilable(String)
  • Added T.must(paths) to ensure Sorbet knows paths is a T::Array[String], not T.nilable(T::Array[String])

This satisfies Sorbet's type checker while preserving the logic. The linter shows no errors, and the type checks should pass in CI.


Open in Cursor Open in Web

Learn more about Cursor Agents

Co-authored-by: martin.emde <martin.emde@gusto.com>
@martinemde martinemde marked this pull request as ready for review November 25, 2025 22:55
Copy link

@ivy ivy left a comment

Choose a reason for hiding this comment

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

Thoughts about deprecating move and making an mv subcommand? I think that would further reinforce the argument order by following Unix conventions.

@ivy ivy moved this from Triage to In Progress in Modularity Dec 30, 2025
@dduugg dduugg requested a review from a team March 5, 2026 17:53
Copy link
Contributor

@dduugg dduugg left a comment

Choose a reason for hiding this comment

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

Thanks for this — the new argument order is more intuitive and aligns nicely with mv.

A few things worth noting before merging:

Breaking change
This is a backwards-incompatible CLI change. Anyone with scripts or tooling using the old destination first order will silently misbehave — their first arg becomes a source path and their last becomes the destination pack. No deprecation warning or migration path is provided. Worth calling this out explicitly and checking the version bump checklist item.

No spec updates visible
There are no test changes in the diff. Existing specs that call move with the old argument order would now be broken. Can you confirm tests pass with the new order, or point to where coverage lives?

StandardError type
Raising StandardError directly prints a backtrace for a user-facing CLI error. Thor::Error (or whatever the existing pattern is in this codebase) would give a cleaner error message without the noise.

Redundant T.must calls
T.must(paths) is unnecessary — a splat result is always an Array, never nil. T.must(pack_name) is also redundant given the args.length < 2 guard above already ensures pack_name is non-nil. Both are harmless but worth cleaning up.

Error message could be more helpful
The current message — 'At least two arguments required: source path(s) and destination pack' — doesn't show the actual usage. Something like Usage: packs move path/to/file.rb [more/paths...] packs/destination would make it easier to self-correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants