Closed MabezDev closed 4 months ago
Hmm I guess without https://github.com/cargo-generate/cargo-generate/issues/884 it makes this quite a bit more difficult, because I means trying to keep rustfmt valid code in snippets :/
I think I like the idea of snippets. We could even check the condition when populating the snippet and wouldn't need the if
in the snippet
While waiting for fmt
support in cargo-generate: What was the reason we removed the cargo fmt
step from the post-script?
While waiting for fmt support in cargo-generate: What was the reason we removed the cargo fmt step from the post-script?
It was causing errors on user who didn't have rustfmt
component installed. We can think adding it back if needed.
I like the snippets idea, it definitely makes the template more clean, specially if we avoid adding includes!
While waiting for fmt support in cargo-generate: What was the reason we removed the cargo fmt step from the post-script?
It was causing errors on user who didn't have
rustfmt
component installed. We can think adding it back if needed.
I wonder if there is a way to ignore such an error or to check if rustfmt
is installed before trying to run the command 🤔
So what's the consenus, do we wait/try to implement cargo generates rustfmt builtin, or do we add back the post creation formatting? If we go with the latter, could someone point me to the PR that removed it as a reference :pray:.
I found at least the PR which originally added cargo fmt
: https://github.com/esp-rs/esp-template/pull/55/files#diff-0faca75bf39fad85951e939d56d340b47f8ff90ccbfb142f6f1787f649c699c4
Probably having it as a built-in feature for cargo-generate
would certainly be even better
I think we need to pass -a
to cargo generate in CI in order to make it execute commands
So the last thing is around the h2, which will probably require changes in esp-wifi. For some reason dhcpv4
is enabled by default, and dhcpv4
enables the wifi
and we get an error.
So the last thing is around the h2, which will probably require changes in esp-wifi. For some reason
dhcpv4
is enabled by default, anddhcpv4
enables thewifi
and we get an error.
I need to compile with "no-default-features" - having "dhcpv4" a default feature there is probably not ideal 🤔
If we add back the rustfmt
cmd, we need to update all the places where we document the cargo-generate
command, formatting the template, in this case, is relatively easy: see https://github.com/esp-rs/esp-template/commit/3623ec5a407f0086cd783f5066684e6fc8974149. But definitely, it's easier with rustfmt
PR is green :sunglasses:, it requires a new release of the esp-wifi, but no rush there. Overall, are we happy with this approach?
I think this approach simplifies the template maintenance! So it's worth giving it a try, if we see that it still hard to maintain in the future we can always reassess.
We just have to take into account that after merging we need to update the books/repos that have the cargo-generrate esp-rs/esp-template
to include the -a
flag.
Edit: We should also update the readme with the -a
flag
I like the approach
Frustratingly cargo generate can't currently do what I want to do for the next step: https://github.com/cargo-generate/cargo-generate/issues/1054#issuecomment-1905835170. I was planning on reusing .github etc for each sub template, but it looks like we might have to duplicate them, at least for now :(. Maybe that's not so bad? We either do that, or maybe we could try and symlinks stuff but that sounds like a recipe for hell on windows.
Oops, completely forgot about this. What do we think about merging this in its current form? At the very least we will get optional formatting on the project at the end which is an improvement.
I can rebase this, and if no one is working on a port to the new release I could also do that too?
It's definitely an improvement so I guess it's worth to merge it
This should be ready to go now I think :)
This PR:
On its own this PR might seem a little pointless, but by having snippets we can easily make the generator less monolithic and easier to maintain. Instead of one template with a million options, we could now easily create more templates with less options which should reduce the cognitive load of making changes. I will do this in a follow up PR because it might be a bit controversial, but I think the changes here are okay on their own.