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

Command to create a new model #1344

Merged
merged 3 commits into from
Nov 14, 2022
Merged

Conversation

MartinKavik
Copy link
Contributor

Resolves #1271

Adds the Clap arg new to create a new model crate, ex.:

cargo run -- --new my_new_model

It creates a new folder my_new_model with the package/crate with the same name. The crate is based on the official example models/star.


Tested on Windows 10 and Kubuntu with cargo run.

The code is based on MoonZoon CLI tool mzoon with removed async support.

Logic: The example models/star (aka template) is packed to a Tar archive and included in fb-app as bytes. Ignored files (hidden, gitignored) and some extra ones are not archived. When the arg --new is processed, the archive is unpacked and files are postprocessed (for instance some text files are modified).


Notes:

  • I didn't want to change Clap logic too much but I agree that something like fb-app model new could be a bit better than --new.
  • Postprocessing currently replaces the relative path to fj to its VERSION_PKG in the new model package. We can add a new flag to don't do that so it's easier to test created packages. It looks like this in mzoon
  • We can consider to make the commands async in the future - I can image some commands like "export_all_models" will need parallelization. The structure might look like this.

P.S. The design & implementation of this feature is quite opinionated. Feel free to close the PR or create a new PR inspired by it. Don't hesitate to copy code from mzoon if it helps.

Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @MartinKavik, this looks great!

I didn't want to change Clap logic too much but I agree that something like fb-app model new could be a bit better than --new.

That's fine. We can always look into improvements later. In addition, there has been a change since I opened #1271 (models are loaded with fj-app my-model instead of fj-app --model my-model), so it might not be practical to add a subcommand-style interface anymore.

Doesn't matter now. We can rethink all of this later.

  • Postprocessing currently replaces the relative path to fj to its VERSION_PKG in the new model package. We can add a new flag to don't do that so it's easier to test created packages. It looks like this in mzoon
  • We can consider to make the commands async in the future - I can image some commands like "export_all_models" will need parallelization. The structure might look like this.

Sounds reasonable 👍

P.S. The design & implementation of this feature is quite opinionated. Feel free to close the PR or create a new PR inspired by it.

I think it looks great, although there might be alternatives (like include_dir). I'd rather merge this now and improve upon it later, than leaving this open while trying to come up with some perfect solution.

Don't hesitate to copy code from mzoon if it helps.

Thank you!

@hannobraun
Copy link
Owner

It looks like this has broken cargo install: #1356

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.

Add command to generate model
2 participants