Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

#9386: Include the rustfmt.toml when generating the node-template #9449

Conversation

ericjmiller
Copy link
Contributor

Fixes #9386
Description
The node template generator now adds the Substrate rustfmt.toml at the root level.

Oh, and I couldn't resist running rustfmt on this file.

Important
I used relative pathing to locate the Substrate rustfmt.toml. Otherwise, it's simple and straightforward.

@ericjmiller ericjmiller requested a review from a team as a code owner July 28, 2021 03:58
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jul 28, 2021

User @ericjmiller, please sign the CLA here.


update_top_level_cargo_toml(&mut cargo_toml, workspace_members, &node_template_path);
}

write_cargo_toml(&t, cargo_toml);
});

// ensures rustfmt is consistent across repositories.
let node_template_rustfmt_toml_path = node_template_path.join("rustfmt.toml");
let rustfmt_target = Path::new("../../rustfmt.toml");
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be relative to options.node_template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @bkchr. Perhaps I misunderstand the issue here.

I thought the options.node_template didn't have the canonical rustfmt.toml and therefore required the Substrate repo's rustfmt.toml. In which case, there isn't a guaranteed way to get to the substrate/rustfmt.toml from the options.node_template path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I believe I understand my confusion. When node-template is referenced in this issue, it's speaking of bin/node-template and therefore exists in this repo. I mistakenly thought it was the devhub node-tempate from another (related) repo passed in as an argument.

Running some QA on the fix and will commit soon.

@bkchr
Copy link
Member

bkchr commented Jul 29, 2021

@ericjmiller can you please merge master again. You seem to have fucked up something 😬

@ericjmiller ericjmiller force-pushed the include-rustfmt.toml-in-node-template branch from ca02c51 to 6acbc73 Compare July 29, 2021 07:34
@ericjmiller
Copy link
Contributor Author

No idea what's going on. My local repo looks fine, then I push to remote and see the large quantity of changes in the PR. very confusing. I may have to just destroy this and recreate.

@ericjmiller ericjmiller deleted the include-rustfmt.toml-in-node-template branch July 29, 2021 17:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include the rustfmt.toml when generating the node-template
2 participants