-
Notifications
You must be signed in to change notification settings - Fork 66
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
Why is the property added collection and not collections? #79
Comments
I'll be honest, I'm pretty stumped on this one. Fiddling with the tests: it('should add the collection property to a file', function(done){
var metalsmith = Metalsmith('test/fixtures/pattern');
metalsmith
.use(collections({
articles: '*.md'
}))
.build(function(err, files){
if (err) return done(err);
console.log(typeof files['three.md']['collection'])
assert.equal(files['three.md'].collection, ['articles']);
done();
});
});
So it is definitely an array. However the test only passes when asserting that collection is a string... EDIT: The tests are passing because assert.equal, which tests "shallow, coercive equality", is being used. It would be very nice to have some clarity here: if the authors of the plugin intended for the collection property on each file to be an array of collection names, an array of collections with files, one string collection name, or, an array of one element - a string containing the collection name... |
It's from when it was originally written. I think the thought process was that the plugin handles collections and each one is a collection. |
@benpolinsky @woodyrew There is actually a very, very compelling reason to do this that I accidentally stumbled upon. Furthermore looking into this made me realize collection is not guaranteed to be an array, in fact if a single collection is added, it will be a string |
This gets me every time I have to grab a collection from a file.
The property that is added to the file is: collection, yet its value is an array of collections
I get this would break everyone's code (unless we add both keys while people migrate), but it really makes zero sense.
Unless I am missing something :)
The text was updated successfully, but these errors were encountered: