Conversation
…rce> support Refactor the compile arguments API to: - Rename `fontc::Args` to `fontc::Options` for clarity (avoids confusion with CLI `args::Args`) - Remove `input` field from Options - source is now passed separately - Update `generate_font()` to accept `Box<dyn Source>` + `Options` - Update `run()` to accept `Input` + `Options` This restores the ability to pass custom `Box<dyn Source>` implementations that was added in #1684, which library users need for generating their own IR programmatically. Also: - Add `Default` derive to `Options` for ergonomic in-memory/WASM usage - Rename `cli_args.rs` back to `args.rs` and delete `compile_args.rs` (Options inlined in lib.rs since it's short enough)
|
before y'all start bikeshedding, here's why I think the name Options beats Args. It's more semantically accured: let args = Args { flags, skip_features: true, ... };
generate_font(source, args) // args to a function that's not main()? |
|
there are precendents in the rust ecosystem e.g. https://doc.rust-lang.org/std/fs/struct.OpenOptions.html |
Change require_dir() to use fs::create_dir_all() instead of fs::create_dir() so that nested paths like /tmp/deep/nested/build are created correctly.
…used
When both --emit-ir and --output-file are used, the font is first
written to {ir_dir}/font.ttf by the persistence mechanism. If output_file
differs, we move (rename) the font to the custom output path rather than
leaving it in the IR directory.
We rename (instead of fs::write or fs::copy) as this matches the current
behavior on `main` whereby the font ends up at output_file only and is not
duplicated in the IR directory.
|
@simoncozens does this work for your use case? |
|
Yes, this works perfectly, thank you! |
cmyr
left a comment
cmyr
left a comment
There was a problem hiding this comment.
One note inline but overall I think the Args/Options distinction is useful, and this looks good :)
| pub fn run(input: Input, options: Options, mut timer: JobTimer) -> Result<(), Error> { | ||
| if options.output_file.is_none() { | ||
| return Err(Error::NoOutputFile); | ||
| } |
There was a problem hiding this comment.
Couldn't this be just a required field on the Args struct, and then Args::parse() will fail immediately?
There was a problem hiding this comment.
the check exists because Options.output_file must be Option<PathBuf> for library/WASM users who don't want file output. Our own main.rs always sets it because it uses Args::try_into() which fills it with .or_else(|| Some(build_dir.join("font.ttf"))), so you may argue it's redundant. However the run() function is pub and someone might call it directly with hand-built Options so the check is a safety net for that case.
There was a problem hiding this comment.
--output-file has an implicit default ("{build_dir}/font.ttf") and the NoOutputFile error only guards against direct run() callers, not CLI users. And we want fontc source.glyphs to continue working as before, without requiring to pass --output-file.
|
thanks for reviewing. I'll let @rsheeter merge this since this goes into his own PR branch, not main |
|
It would have been nice to rename separate of the other changes because it's so noisy but lgtm |
4 participants
Refactor the compile arguments API to:
fontc::Argstofontc::Optionsfor clarity (avoids confusion with the clap-based CLIargs::Args)inputfield from Options - source is now passed separatelygenerate_font()to acceptBox<dyn Source>+Optionsrun()to acceptInput+OptionsThis restores the ability to pass custom
Box<dyn Source>implementations that was added in #1684, which library users need for generating their own IR programmatically.Also:
Defaultderive toOptionsfor ergonomic in-memory/WASM usagecli_args.rsback toargs.rsand deletecompile_args.rs(Options inlined in lib.rs since it's short enough)