Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global command to the debugger #142

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

k88hudson-cfa
Copy link
Collaborator

This PR adds a global command to the debugger, which prints the value of a registered global property to the console.

If you type global (with no argument) or with an invalid property name, you will see a list of available properties:

t=0 $ global
2 global properties registered:
runner.FavoriteNumber, runner.Name

If you provide a valid, fully qualified property name, you will see the value:

t=0 $ global runner.Name
"Sim123"

@k88hudson-cfa k88hudson-cfa requested a review from ekr-cfa January 3, 2025 19:16
src/debugger.rs Outdated
format!(
"{} global properties registered:\n{}",
properties.len(),
properties.join(", ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is going to happen when this is super-long? Are you just relying on the console wrapping?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I switched this to newline

properties.join(", ")
)
}
impl DebuggerCommand for GlobalPropertyCommand {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So what is your plan for the syntax for global set?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd still like to understand this point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a document with some ideas about this

src/debugger.rs Outdated Show resolved Hide resolved
src/global_properties.rs Show resolved Hide resolved
src/global_properties.rs Show resolved Hide resolved
src/global_properties.rs Show resolved Hide resolved
"{} global properties registered:",
context.list_registered_global_properties().len()
);
assert!(output.into_string().contains(&expected));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason for .contains() rather than equals?

Copy link
Collaborator Author

@k88hudson-cfa k88hudson-cfa Jan 4, 2025

Choose a reason for hiding this comment

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

It's sort of hard to know what all the registered global properties actually are in tests; I switched the other contains though

src/debugger.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ekr-cfa ekr-cfa left a comment

Choose a reason for hiding this comment

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

Still need to review the code, but left some comments on the doc.

of a primary command, subcommands, and arguments:

```
<primary_command> [<subcommand>] [<positional_argument>] [--<argument> [<value>]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't positional arguments usually come after named arguments?

- `<primary_command>` is the top-level command category (e.g., `breakpoint`, `global`, `query`, `people`).
- `[<subcommand>]` specifies an action to perform (e.g., `set`, `get`, `list`, `delete`).
- `[<positional_argument>]` may be used to provide necessary inputs (e.g., the name of a property)
- `[--<argument> [<value>]]` represents named options that modify behavior, or provide additional inputs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not --argument=value


Set a breakpoint at the specified time.

Example: `breakpoint set --time 4.0`
Copy link
Collaborator

Choose a reason for hiding this comment

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

What could there be besides time here?


Set the specified global variable to a value.

Example: `global set ixa.Foo=42`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good syntax but is it going to challenge clap?


List all global variables with the specified prefix.

Example: `global list --prefix ixa.`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice one.


Example: `people set --id 42 --property Region=CA`

- **`people add [--properties Region=CA]`**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you need --properties here. What else could be there?



### 4. Queries
Commands for creating pre-saved queries of people by property-value pairs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think we should call these people query


- **`query get <id>`**

Retrieve a query by identifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

This just prints it out?

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