Skip to content

Conversation

@Toilal
Copy link

@Toilal Toilal commented Apr 1, 2017

Close #427

@kazupon
Copy link
Member

kazupon commented Apr 5, 2017

/ping @egoist @zigomir

@zigomir
Copy link
Contributor

zigomir commented Apr 6, 2017

I think this would be even better with tests.

@Toilal
Copy link
Author

Toilal commented Apr 6, 2017

I'll write one test that writes target files to another destination, as I think it would be a common use case.

@Toilal
Copy link
Author

Toilal commented Apr 6, 2017

Test written. I have also removed it.only in test.js that was causing only one test to run.

expect(getTemplatePath('../template')).to.equal(path.join(__dirname, '/../../../template'))
})

it.only('points out the file in the error', done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@zigomir zigomir left a comment

Choose a reason for hiding this comment

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

Looking good. Still it would be even better if test code exercises more paths like for before and after.

delete files['readme.md']
files['custom/readme.md'] = readme

console.log(files)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this could be removed.

Copy link
Author

Choose a reason for hiding this comment

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

oups yes :)


var readme = files['readme.md']
delete files['readme.md']
files['custom/readme.md'] = readme
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be sure. What you're doing here in custom metalsmith plugin is you're moving file from root template directory into custom directory, right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's it.

@@ -0,0 +1,17 @@
module.exports = {
"metalsmith": function (metalsmith, opts, helpers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Besides this one you can also test before and after, right?

Copy link
Author

Choose a reason for hiding this comment

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

ok

Copy link
Author

Choose a reason for hiding this comment

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

I just push a new commit with an additional test, and better assertions

@Toilal Toilal force-pushed the feature/metalsmith-builder-hooks branch from 0a8c6a7 to 0125ac7 Compare April 7, 2017 18:58
@zigomir
Copy link
Contributor

zigomir commented Apr 9, 2017

@Toilal looks good, thank you!

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.

4 participants