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

Allow access to metadata if needed #4

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

Conversation

mac2000
Copy link

@mac2000 mac2000 commented Aug 8, 2015

Did not found a way to access metadata, so here is my PR

Unfortunatelly did not found a way to pass metadata for callbacks with 3 parameters without braking everything.

Also probably there is a way to replace this for function but I bet it will brake someones code, so probably the only one way left is to use callback with four parameters and call done every time.

@wilsaj
Copy link
Owner

wilsaj commented Aug 8, 2015

Thanks for the PR 👍! This is a good point. I have two thoughts / suggestions.

  1. I agree that there isn't a really a good way to handle three parameters without introducing breaking changes. But I'm ok with breaking changes if we bump the version number and document the changes in the README so they are easy to find. The npm semver rules allow for breaking changes in a way that (usually) doesn't mess everyone else up.

    So, as an alternative, how do you think about passing metadata as the third argument always, and the 4-argument version (file, files, metadata, done) becomes the only way to do async?

  2. Another thing to consider: how do you feel about passing the entire metalsmith object instead of just metadata? So accessing metadata looks like this:

      Metalsmith(files)
        .metadata({foo: 'bar'})
        .use(each(function(file, filename, metalsmith, done){
          assert.equal(metalsmith.metadata().foo, 'bar');
          done();
        }))
    

    I slightly prefer this because it is more general and works exactly the same as the metalsmith plugins API.

@mac2000
Copy link
Author

mac2000 commented Aug 9, 2015

Yes it seems that you are right in both cases.

The only one thing I have found yesterday still missing for me is the need to filter file types, so for example I want to add title property for all markdown and html files but not images.

So if there will be breaking changes may be there is a chance that we will see also something like:

each(["**/*.md", "**/*.html"], file, file name, metalsmith, done)

But this is not required, it saves only one if statement inside each function

@wilsaj
Copy link
Owner

wilsaj commented Aug 9, 2015

Have you looked at metalsmith-branch? Would that work?

@mac2000
Copy link
Author

mac2000 commented Aug 9, 2015

Ohh, definitely thank you for that.

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.

2 participants