Skip to content

Conversation

@joshi4
Copy link
Contributor

@joshi4 joshi4 commented Oct 28, 2025

Clarify required vs optional for sf nodes options

@semanticdiff-com
Copy link

semanticdiff-com bot commented Oct 28, 2025

Review changes with  SemanticDiff

Changed Files
File Status
  src/lib/nodes/utils.ts  11% smaller
  src/lib/nodes/create.ts  0% smaller
  src/lib/nodes/extend.ts  0% smaller

Copy link
Contributor Author

joshi4 commented Oct 28, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Improved CLI user experience by clarifying which options are required vs optional for sf nodes commands.

  • Added [Required] prefix labels to mandatory options (--zone, --max-price, --duration)
  • Added [Required: names or --count] to clarify mutual requirement in sf nodes create
  • Added ISO 8601 format examples (e.g., 2022-10-27T14:30:00Z) to time-related options
  • Added Notes section in help text explaining option requirements and relationships
  • Fixed node ID format in example (updated to use correct n_ prefix)
  • Minor formatting improvements (removed trailing whitespace)

Confidence Score: 5/5

  • This PR is safe to merge with no risk
  • This PR only contains documentation and help text improvements with no changes to business logic or code behavior. The changes improve user experience by making CLI option requirements clearer, which reduces user errors. The lockfile update is standard and automated.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
src/lib/nodes/utils.ts 5/5 Added [Required] labels to option descriptions for zone, max-price, and duration; clarified ISO 8601 format examples
src/lib/nodes/create.ts 5/5 Added [Required: names or --count] labels and notes section clarifying option requirements; fixed minor formatting issue
src/lib/nodes/extend.ts 5/5 Updated example to use correct node-id format and applied formatting changes

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI
    participant Commander
    participant Help System
    
    User->>CLI: Execute `sf nodes create --help`
    CLI->>Commander: Parse command and options
    Commander->>Help System: Display help text
    
    Note over Help System: Shows option descriptions with<br/>[Required] labels for clarity
    
    Help System-->>User: Display help with clarified requirements
    
    Note over User: User sees:<br/>- "[Required] Zone for your nodes"<br/>- "[Required] Maximum price per node hour"<br/>- "[Required: names or --count]"<br/>- ISO 8601 format examples
    
    User->>CLI: Execute command with options
    CLI->>Commander: Validate required options
    
    alt Missing required option
        Commander-->>User: Error: required option missing
    else All required options present
        Commander->>CLI: Execute node operation
        CLI-->>User: Success
    end
Loading

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@joshi4 joshi4 force-pushed the clarify-nodes-create-flags branch from dbd8da6 to 6110372 Compare October 28, 2025 02:21
Copy link
Contributor Author

joshi4 commented Oct 28, 2025

before

image.png

after
image.png

@sigmachirality
Copy link
Member

Nice! I might work off this and have cursor take a stab at doing this for all commands.

@sigmachirality
Copy link
Member

@cursoragent similar to existing changes add [Required] or [Required: x or <--other-option]

@cursor
Copy link

cursor bot commented Oct 28, 2025

Perfect! I've successfully applied the [Required] pattern to all commands in the CLI, similar to the existing changes in PR #221. Here's what I updated:

Changes Made

1. sf buy command (src/lib/buy/index.tsx)

Added [Required: --type or --zone or --colocate] to:

  • --type option
  • --zone option
  • --cluster option
  • --colocate option

Added [Required: --duration or --end] to:

  • --duration option
  • --end option

2. sf scale update command (src/lib/scale/update.tsx)

Added [Required: at least one of --accelerators, --horizon, or --price] to:

  • --accelerators option
  • --horizon option
  • --price option

Already Properly Marked

The following commands already use requiredOption() which automatically marks them as required in help text:

All other commands (contracts, orders, zones, balance, login, me, etc.) either don't have required options or their requirements are simple enough that they don't need special marking.


Open in Cursor Open in Web

Learn more about Cursor Agents

@cursor
Copy link

cursor bot commented Oct 28, 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

Copy link
Contributor Author

joshi4 commented Oct 28, 2025

@sigmachirality feel free to merge whenever you're happy

@sigmachirality
Copy link
Member

@cursoragent commander requiredOption and Option.makeRequired do not actually mark the help text with the "required" text. Add [Required] like the sf nodes command

@cursor
Copy link

cursor bot commented Oct 30, 2025

Perfect! I've successfully added [Required] markers to all options that use requiredOption() or makeOptionMandatory(). Here's a complete summary of the changes:

Files Updated

Commands using requiredOption() - Now with [Required] markers:

  1. src/lib/scale/create.tsx

    • --accelerators[Required] Desired number of GPUs (0 to turn off)
  2. src/lib/extend/index.tsx

    • --contract[Required] Contract ID to extend
    • --duration[Required] Extension duration
  3. src/lib/sell.ts and src/lib/sell/index.tsx

    • --price[Required] The price in dollars, per GPU hour
    • --contract-id[Required] Specify the contract ID
  4. src/lib/nodes/image/upload.ts

    • --file[Required] Path to the image file
    • --name[Required] Image name
  5. src/lib/tokens.ts

    • --id[Required] Specify the token ID
  6. src/lib/clusters/clusters.tsx

    • --cluster[Required] name of the cluster
    • --user[Required] Username to add
  7. src/lib/vm/logs.ts

    • --instance[Required] VM instance ID
  8. src/lib/vm/replace.ts

    • --id[Required] ID of the VM to replace
  9. src/lib/vm/script.ts

    • --file[Required] Path to startup script file

Already properly marked (from PR #221 and my earlier changes):

  • src/lib/nodes/utils.ts - Options using makeOptionMandatory()
  • src/lib/buy/index.tsx - Conditional requirements like [Required: --type or --zone or --colocate]
  • src/lib/scale/update.tsx - Conditional requirements

All required options now have explicit [Required] markers in their help text, making it clear to users which options are mandatory! 🎉


Open in Cursor Open in Web

Learn more about Cursor Agents

@joshi4 joshi4 force-pushed the clarify-nodes-create-flags branch from 2fb28d9 to ca33878 Compare November 4, 2025 18:21
Copy link
Contributor Author

joshi4 commented Nov 4, 2025

only updates the two relevant nodes commands.

dtao okayed this merge in slack

@joshi4 joshi4 merged commit b41e293 into main Nov 4, 2025
2 checks passed
@sigmachirality
Copy link
Member

the notes section is a nice touch

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.

3 participants