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

. #3

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

. #3

wants to merge 7 commits into from

Conversation

jeapostrophe
Copy link
Contributor

No description provided.

tools/generator.js Outdated Show resolved Hide resolved
return `Error writing ${cfgPath}`;
}
// Read config.json.
configJson = await fs.readJson(cfgPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you read this after you just wrote it up on line 287 and then you write it again later? Why not just keep it in memory?

Copy link
Contributor

@hagenhaus hagenhaus Nov 3, 2021

Choose a reason for hiding this comment

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

Yes, you are right. I should keep it in memory. One of the plugins (copyFmToConfig) needs to update configJson (overwriting default values with frontmatter as needed) so, at the time, I wrote configJson to a file so that the plugin could read it, update it, and write it again. It might be better to just pass the configJson object to the plugin. This particular plugin still uses readJsonSync instead of async(), too. Would you be interested in making these changes as part of the PR? The plugins are in the tools/plugins folder. Thx.

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.

2 participants