Minimal implementation of sys.inputs by edhebi · Pull Request #2481 · typst/typst

edhebi · 2023-10-25T01:29:31Z

Fixes #295.

This isn't prod ready by any mean but I felt like I might as well push a minimal impl to gather some discussions.

Things of interest:

This changes the signature of typst_library::build() which is fairly disruptive, but I feel it's ok given the fix is just adding Default::default.
This makes the sys module a bit special in typst_library as the arguments of build are explicitly named after it. I feel this is a good way to ensure going through the World stays the main way to handle platform-specific things, while sys becomes kind of its own thing.
I didn't see much discussion on the actual cli option so I went for a basic syntax to pass named values, and a more general purpose json syntax to make scripting nicer. the json syntax is currently the only way to define non-string values in the cli
This allocates all over the place, partly because I wasn't sure about whether I should bring EcoString in args.rs
There's no handling whatsoever of what happens when you have multiple inputs with the same key. currently it takes the latest in a semi arbitrary order, but maybe we want to offer more garantees, and at least we should diagnose it
I have no idea how to do testing for this, suggestions welcome :)

Keeping this as a draft for now, at least until the first wave of feedback

mattfbacon

In regards to the Default::default comments, I think there could be two possible ways to improve the experience:

Create a new function, build_with_sys, or the inverse build_without_sys (bikeshed names)
Use a builder pattern like library::build().with_sys_args(sys_args).

mattfbacon · 2023-10-25T18:13:48Z

crates/typst-docs/src/lib.rs


 static LIBRARY: Lazy<Prehashed<Library>> = Lazy::new(|| {
-    let mut lib = typst_library::build();
+    let mut lib = typst_library::build(Default::default());


Clippy doesn't like Default::default. It wants you to include the actual type name.

Really ? that's a bit silly. I didn't get that lint, did you do something special to get it ?

https://rust-lang.github.io/rust-clippy/stable/index.html#/default_trait_access

From that link, seems like it is restricted to when pedantic lints are enabled. Typst doesn't enable those on CI, so I'm not sure we should follow those.

With that said, changing to SysArguments::default() probably would aid readability anyway.

probably would aid readability anyway

That's the idea of pedantic lints. The code does exactly the same thing, just a bit clearer because people may be confused about "the default of what?" when they see this.

mattfbacon · 2023-10-25T18:14:33Z

crates/typst-library/src/sys.rs

+    scope.category("sys");
+    scope.define(
+        "version",
+        Version::from_iter([
+            env!("CARGO_PKG_VERSION_MAJOR").parse::<u32>().unwrap(),
+            env!("CARGO_PKG_VERSION_MINOR").parse::<u32>().unwrap(),
+            env!("CARGO_PKG_VERSION_PATCH").parse::<u32>().unwrap(),
+        ]),
+    );
+    scope.define(
+        "inputs",
+        Dict::from_iter(args.inputs.into_iter().map(|(k, v)| (k.into(), v))),
+    );


This is a small point, but I have observed that you can get smaller and nicer output from rustfmt by doing something like

let inputs = Dict::from_iter(...); scope.define("inputs", inputs);

I think it's fine this way, since scope.define("inputs", some_expression) is already kinda saying something like inputs = some_expression, so it's readable.

At most, I could see adding some comment like "// converting keys to EcoString" or something like that (?). Or the .into() could be changed to something like FinalType::from(k).

It's not for clarity, just for formatting.

mattfbacon · 2023-10-25T18:14:42Z

tests/src/benches.rs


        Self {
-            library: Prehashed::new(typst_library::build()),
+            library: Prehashed::new(typst_library::build(Default::default())),


Ditto about Default::default

mattfbacon · 2023-10-25T18:14:55Z

tests/src/tests.rs

    }

-    let mut lib = typst_library::build();
+    let mut lib = typst_library::build(Default::default());


Ditto about Default::default

mattfbacon · 2023-10-25T18:16:04Z

crates/typst-cli/src/world.rs

+        let mut inputs = HashMap::with_capacity(command.plain_inputs.len());
+        for (k, v) in &command.plain_inputs {
+            inputs.insert(k.clone().into(), Value::Str(v.clone().into()));
+        }
+        for raw in &command.json_inputs {
+            let root: HashMap<EcoString, Value> =
+                serde_json::from_str(raw).map_err(|e| eco_format!("{e}"))?;
+            inputs.reserve(root.len());
+            for (key, val) in root {
+                inputs.insert(key, val);
+            }
+        }


This could be done more idiomatically with iterators.

Iterators get kind of annoying here because from_iter requires annotations to deduce the RandomState type, but I'll try to see if I can find a nicer way

That's strange. I'm surprised you can't just do .collect::<HashMap<EcoString, Value>>()

That's strange. I'm surprised you can't just do .collect::<HashMap<EcoString, Value>>()

I believe it should be possible (without using Dict::from_iter):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=7007968ee2585f2a7e800087bd5e9d7f

.collect() is a very powerful beast in Rust (see also a cool fun fact about .collect()).

That collecting iterator of Results to Result of a container thing will be necessary here considering the possibility of failure for the JSON deserialization.

mattfbacon · 2023-10-25T18:16:19Z

crates/typst-cli/src/world.rs

+        let input = command.input_file.canonicalize().map_err(|_| {
+            eco_format!(
+                "input file not found (searched at {})",
+                command.input_file.display()


Trailing comma

mattfbacon · 2023-10-25T18:19:31Z

crates/typst-cli/src/args.rs

+        short = 'i',
+        value_name = "KEY=VALUE",
+        action = ArgAction::Append,
+        value_parser = ValueParser::new(parse_input_pair)


Trailing comma

laurmaedje · 2023-10-31T19:55:11Z

crates/typst-library/src/sys.rs

+pub struct SysArguments {
+    /// A number of keyed inputs that can be provided by the platform and will appear
+    /// as `sys.inputs`. The main expected usecase is scripting.
+    pub inputs: HashMap<EcoString, Value>,


I think it could already be a Dict at this point.

laurmaedje · 2023-10-31T19:55:33Z

crates/typst-library/src/sys.rs

+
+/// Hook up all sys definitions.
+pub(super) fn define(global: &mut Scope, args: SysArguments) {
+    global.category("system");


Suggested change

global.category("system");

global.category("sys");

laurmaedje · 2023-10-31T19:56:45Z

crates/typst-library/src/sys.rs

+
+/// Arguments for the `sys` module that handle implementation-specific behaviour.
+#[derive(Clone, Default)]
+pub struct SysArguments {


If we have this struct, maybe we could actually allow overriding the version here and keep the default as the Cargo.toml thing?

laurmaedje · 2023-10-31T19:58:27Z

crates/typst-cli/src/args.rs

+        value_name = "JSON",
+        action = ArgAction::Append,
+    )]
+    pub json_inputs: Vec<String>,


I'm not sure I like the json inputs thing. I'd like to keep the API surface minimal and using a single normal input in combination with json.parse can already achieve the same thing (and is not format-specific).

laurmaedje · 2023-10-31T19:59:53Z

crates/typst-cli/src/args.rs

+    // The key was missing or entirely blank
+    MissingKey,
+    // The value was missing or entirely blank
+    MissingValue,


Is passing the empty string not a valid thing?

laurmaedje · 2023-10-31T19:59:57Z

crates/typst-cli/src/args.rs

+    #[clap(
+        long = "input",
+        short = 'i',
+        value_name = "KEY=VALUE",


I'm not super familiar with typical command line argument conventions. Is this the canonical way to deal with multiple key-value inputs?

laurmaedje · 2023-10-31T20:01:53Z

crates/typst-cli/src/args.rs

 #[derive(Debug, Clone, Args)]
 pub struct SharedArgs {
    /// Path to input Typst file
-    pub input: PathBuf,


the collision between the names here is a bit unfortunate ...

Maybe "source" as the new name?

laurmaedje · 2023-10-31T20:03:57Z

This allocates all over the place, partly because I wasn't sure about whether I should bring EcoString in args.rs

args.rs should not depend on typst because it's used in the build script. I think just String is ok here.

laurmaedje · 2023-11-27T10:40:51Z

@edhebi do you still plan on working on this? if not, someone else could pick it up since it's almost done.

laurmaedje · 2023-12-13T10:35:39Z

Closing in favor of #2894 due to inactivity.

minimal implementation of sys.inputs

818445c

mattfbacon reviewed Oct 25, 2023

View reviewed changes

laurmaedje reviewed Oct 31, 2023

View reviewed changes

laurmaedje added the waiting-on-author Pull request waits on author label Nov 17, 2023

C0ffeeCode mentioned this pull request Dec 9, 2023

Key/Value data from CLI #2894

Merged

laurmaedje closed this Dec 13, 2023

tingerrr mentioned this pull request Dec 31, 2023

Theme-dependent default color value, so packages don't use hardcoded black as the default line/fill color #3104

Closed

laurmaedje removed the waiting-on-author Pull request waits on author label Dec 18, 2025

Uh oh!

Conversation

edhebi commented Oct 25, 2023

Uh oh!

mattfbacon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PgBiel Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurmaedje Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurmaedje Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurmaedje commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurmaedje commented Nov 27, 2023

Uh oh!

laurmaedje commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PgBiel Oct 26, 2023 •

edited

Loading

laurmaedje Oct 31, 2023 •

edited

Loading

laurmaedje Oct 31, 2023 •

edited

Loading

laurmaedje commented Oct 31, 2023 •

edited

Loading