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
Pass globals as command-line options #78
Changes from all commits
96bd7df
ea50f89
65b2abb
66ff6b8
23acf80
2dd6bc7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consequence of bumping |
||
.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), | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Also, if you want to bound the region where let globals_ = globals_; after you're done mutating it. That makes it immutable for the remainder of the scope.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Cool, if this is idiomatic then I'll be happy to
Oh, that's a lovely use of shadowing! Thank you. |
||
|
||
let config = Config::load()?; | ||
let mut loader = Loader::new()?; | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
let graph = file | ||
.execute(&tree, &source, &mut config) | ||
.with_context(|| format!("Cannot execute TSG file {}", tsg_path.display()))?; | ||
|
There was a problem hiding this comment.
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.