-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat(silo): Initial Silo scaffolding #2096
base: main
Are you sure you want to change the base?
Conversation
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.
Looks good to me.
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.
This is great code to read and very comprehensible. A great start to Silo!
|
||
let build_config = { | ||
elide_type_info: | ||
get(config, build |-- key("elide-type-info") |-- bool), |
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.
I'm curious about the choice of kebab case? The convention within the language itself is camel case so why not use that here?
Afaik there's no key naming convention in TOML and I've seen a mixture of all of
- Rust, Gleam: snake case
- Python: kebab case (odd that it's not snake case...)
- Hugo: camel case
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.
Rust uses kebab case, so that + mostly translating command line flags is what got me here. I'd be open to camel case though. I sort of like the configuration language being different than the source language though.
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.
Ah yeah rust does use kebab case, my bad. Same question about package names and output binary name conventions: I see in the PR description you have kebab case but in the stdlib the convention so far has been camel case. When package management, etc rolls out are we going with kebab case package names like npm?
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.
Why is silo a separate cli tool rather than being integrated into the grain
command? E.g. grain build
, grain new
?
~usage="[options] <path>", | ||
~options=[name_opt], | ||
~args=[path_arg], | ||
Cmd.Thunk(() => New.new_(~name=?name^, path^)), |
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.
What do you think about the path optional at which point the current dir will be used?
Silo is meant to completely replace the |
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.
Left a few comments but looks great overall!
); | ||
Printf.fprintf(oc, "module Main\n\nprint(\"Hello, world!\")\n"); | ||
close_out(oc); | ||
}; |
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.
Maybe can report a cheesy feedback message like "New grain project template created at [path]. Happy coding! 🌾"
|
||
let build_config = { | ||
elide_type_info: | ||
get(config, build |-- key("elide-type-info") |-- bool), |
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.
Ah yeah rust does use kebab case, my bad. Same question about package names and output binary name conventions: I see in the PR description you have kebab case but in the stdlib the convention so far has been camel case. When package management, etc rolls out are we going with kebab case package names like npm?
|
||
{package: package_config, build: build_config, bin: bin_config}; | ||
| `Error(msg, loc) => raise(LoadError(ConfigParseError(msg, loc))) | ||
| exception (Sys_error(_)) => raise(LoadError(ConfigNotFound)) |
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.
Should parent directories also be looked through?
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.
I feel like it would make sense for us to look through parent directories, (especially if we are using silo.toml
for the lsp as you might open vscode on a nested library and want your settings to follow over), I think if we do this though it would be beneificial to have a message like Building <path> project
or just something to indicate which config is being used.
Cmd.make( | ||
~name="build", | ||
~doc="Compile the current project", | ||
~usage="[options]", |
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.
What do you think about having an optional argument to specify the directory to build from to avoid having to cd
into it? So if you had /a/b/silo.toml
and you're in /a
you could do silo build ./b
let profile = ref(Build.Dev); | ||
let release_opt = | ||
Cmd.flag( | ||
~names=["--release"], |
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.
Why is this a CLI flag rather than something in the config TOML?
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.
I assume this is a cli flag over being in the config because the average workflow would be compiling normally and then release
is only used when your building for production at the end.
What if instead of having this as a cli flag though we had build profiles in the config. so you could silo build dev
or silo build release
, (We could autogenerate a dev and release build, and have the regular build be the development one), this would also help to accomodate multiple build targets.
| None => raise(NewError(MustProvideName(path))) | ||
} | ||
}; | ||
|
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.
Should we check if the new project your trying to make already exists and provide a warning? (Maybe we can allow it to be overridden with a force argument if needed).
This PR does some initial project scaffolding for Silo, Grain's new project manager and build system. This isn't quite an initial version, but rather some project setup to provide a foundation for us to work from as we build out functionality. It's not yet hooked up to our release process to allow us to continue Grain releases while we develop Silo in tandem.
Two commands are present,
new
andbuild
:Silo will eventually manage build artifacts itself rather than offload that functionality to
grainc
, manage dependencies (including the standard library), run wasm files, and more (though not necessarily all of these before an initial release).As Silo does not yet handle dependencies/stdlib,
build.stdlib
must be set in yoursilo.toml
to build projects.I put together a Silo README as a basis for our Silo documentation which includes the following
silo.toml
configuration information, but I put it here in the PR description as well for easy access.