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.

Sign up for GitHub

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

Jump to bottom

Pass globals as command-line options #78

Merged
merged 6 commits into from Aug 4, 2022
Merged

Pass globals as command-line options #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
96bd7df
Bump clap to 3.2.
robrix Jul 28, 2022
ea50f89
Match zero or more --global options.
robrix Jul 28, 2022
65b2abb
Split globals on =.
robrix Jul 28, 2022
66ff6b8
Collect a vector of key-value globals.
robrix Jul 28, 2022
23acf80
Construct Globals from the CLI.
robrix Jul 28, 2022
2dd6bc7
Pass the globals along to execution.
robrix Jul 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@ cli = ["clap", "env_logger", "tree-sitter-config", "tree-sitter-loader"]

[dependencies.clap]
optional = true
version = "2.32"
version = "3.2"
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to bump this to be able to supply the args the way I wanted.


[dependencies.env_logger]
optional = true
Expand Down
29 changes: 24 additions & 5 deletions src/bin/tree-sitter-graph/main.rs
View file
Open in desktop
Expand Up @@ -10,14 +10,17 @@ use std::path::Path;
use anyhow::anyhow;
use anyhow::Context as _;
use anyhow::Result;
use clap::builder::ArgAction;
use clap::App;
use clap::Arg;
use tree_sitter::Parser;
use tree_sitter_config::Config;
use tree_sitter_graph::ast::File;
use tree_sitter_graph::functions::Functions;
use tree_sitter_graph::graph;
use tree_sitter_graph::parse_error::ParseError;
use tree_sitter_graph::ExecutionConfig;
use tree_sitter_graph::Identifier;
use tree_sitter_graph::Variables;
use tree_sitter_loader::Loader;

Expand All @@ -36,21 +39,21 @@ fn main() -> Result<()> {
.arg(Arg::with_name("source").index(2).required(true))
.arg(
Arg::with_name("quiet")
.short("q")
.short('q')
Comment on lines -39 to +42
Copy link
Member Author

Choose a reason for hiding this comment

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

Consequence of bumping clap.

.long("quiet")
.help("Suppress console output"),
)
.arg(
Arg::with_name("lazy")
.short("z")
.short('z')
.long("lazy")
.help("Use lazy evaluation (experimental)"),
)
.arg(Arg::with_name("scope").long("scope").takes_value(true))
.arg(Arg::with_name("json").long("json").takes_value(false))
.arg(
Arg::with_name("output")
.short("o")
.short('o')
.long("output")
.requires("json")
.takes_value(true),
Expand All @@ -60,13 +63,30 @@ fn main() -> Result<()> {
.long("allow-parse-errors")
.takes_value(false),
)
.arg(
Arg::with_name("global")
.long("global")
.takes_value(true)
.action(ArgAction::Append),
)
.get_matches();

let tsg_path = Path::new(matches.value_of("tsg").unwrap());
let source_path = Path::new(matches.value_of("source").unwrap());
let current_dir = std::env::current_dir().unwrap();
let quiet = matches.is_present("quiet");
let lazy = matches.is_present("lazy");
let globals = matches.get_many::<String>("global").unwrap_or_default();
let mut globals_ = Variables::new();
for kv in globals {
let kv_ = kv
.split_once('=')
.with_context(|| format!("Expected key-value pair separated by '=', got {}.", kv))?;
globals_.add(
Identifier::from(kv_.0),
graph::Value::String(kv_.1.to_string()),
)?;
}
Comment on lines +79 to +89
Copy link
Member Author

Choose a reason for hiding this comment

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

I would have preferred to do this with a single map but I couldn't figure out how to express the two nested levels of failure (failure in parsing global values, and failure in the surrounding context). If anyone has suggestions, I'm all ears; I think we should be able to avoid both the second variable and the mutability, and (ideally) end up with something more readable as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually like how you've done it here! As you point out, it becomes messy with the two levels of error handling, and I think this rendering makes the control flow clearer.

(It's doable, and probably involves the collect method, which takes a vec of results and turns it into a result of a vec. But again, I'd rather we keep it as you've written it here.)

Copy link
Contributor

Choose a reason for hiding this comment

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

(Also, if you want to bound the region where globals_ is mutable, you can do something like this:

let globals_ = globals_;

after you're done mutating it. That makes it immutable for the remainder of the scope.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually like how you've done it here! As you point out, it becomes messy with the two levels of error handling, and I think this rendering makes the control flow clearer.

(It's doable, and probably involves the collect method, which takes a vec of results and turns it into a result of a vec. But again, I'd rather we keep it as you've written it here.)

Cool, if this is idiomatic then I'll be happy to :shipit:

(Also, if you want to bound the region where globals_ is mutable, you can do something like this:

let globals_ = globals_;

after you're done mutating it. That makes it immutable for the remainder of the scope.)

Oh, that's a lovely use of shadowing! Thank you.


let config = Config::load()?;
let mut loader = Loader::new()?;
Expand Down Expand Up @@ -116,8 +136,7 @@ fn main() -> Result<()> {
}

let mut functions = Functions::stdlib();
let globals = Variables::new();
let mut config = ExecutionConfig::new(&mut functions, &globals).lazy(lazy);
let mut config = ExecutionConfig::new(&mut functions, &globals_).lazy(lazy);
Comment on lines -120 to +139
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the key to how this all works: instead of passing in an empty map of Variables, we pass in the one we built above instead. Really quite tidy, that.

let graph = file
.execute(&tree, &source, &mut config)
.with_context(|| format!("Cannot execute TSG file {}", tsg_path.display()))?;
Expand Down