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

Pass answers to completion message template #176

Closed
wants to merge 2 commits into from
Closed

Pass answers to completion message template #176

wants to merge 2 commits into from

Conversation

kjarmicki
Copy link
Contributor

This will allow us to come up with better completion messages based on user answers.

@@ -54,7 +53,7 @@ export default function generateTemplate(name, src, dest, done) {
}
log.success('Project created');
done();
logMessage(opts.completionMessage, data);
logMessage(opts.completionMessage, { data, ...metalsmith.metadata() });
Copy link

Choose a reason for hiding this comment

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

Should it be ...data instead of data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, totally! Otherwise it just gets destructured as { data: data }. Nice catch, thx :)

@@ -54,7 +53,7 @@ export default function generateTemplate(name, src, dest, done) {
}
log.success('Project created');
done();
logMessage(opts.completionMessage, data);
logMessage(opts.completionMessage, { ...data, ...metalsmith.metadata() });
Copy link
Member

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 flipping the order on these? To make sure that destDirName and inPlace are consistent, even if they also are one of the answers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but I'd lean more towards validating keys in prompts as soon as they come and making sure that destDirName and inPlace are not used, because otherwise we end up with some confusion ("I've declared my inPlace and it's a different value in completionMessage and in rendered template").
Early error "inPlace is a reserved property" may help us avoid it.

Copy link
Member

@dlmr dlmr May 19, 2017

Choose a reason for hiding this comment

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

I have now looked into this some more I have found one bug/incorrect behavior, and one regression from #171.

I think we should actually mutate metalsmith.metadata(), something that seems like a somewhat normal pattern and it would make it possible to read inPlace and destDirName in the questions as well. Additionally noEscape is ignored after #171, and should actually me respected always as it was before.

I think we should either manage this in this PR changing the purpose of it or (maybe better) close this PR and open a new. The changes done here should I think be considered a bug, and not a lack of feature. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, seems reasonable :) I'll take a closer look at it on monday

@kjarmicki
Copy link
Contributor Author

Closing this one and will open subsequent to address regression caused by #171 first

@kjarmicki kjarmicki closed this May 22, 2017
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.

3 participants