search

DamonOehlman / filestream

W3C File Reader API streaming interfaces
30 stars 11 forks source link

add 'meta' option to emit metadata in stream #5

Closed feross closed 10 years ago

feross commented 10 years ago

This is a great module! Thanks for making it.

One thing that tripped me when I was getting started with this module is that it emits the file's metadata into the stream. I think this is the wrong default behavior because it violates the 'principle of least astonishment'.

This PR turns off this behavior by default, and adds a meta option to re-enable it.

What do you think?

(Depends on PR #4)

DamonOehlman commented 10 years ago

Hey mate, I'll merge all your PRs through soon. Just on holidays at the moment :) From: Feross AboukhadijehSent: Tuesday, 8 July 2014 15:27To: DamonOehlman/filestreamReply To: DamonOehlman/filestreamSubject: [filestream] add 'meta' option to emit metadata in stream (#5)This is a great module! Thanks for making it.

One thing that tripped me when I was getting started with this module is that it emits the file's metadata into the stream. I think this is the wrong default behavior because it violates the 'principle of least astonishment'.

This PR turns off this behavior by default, and adds a meta option to re-enable it.

What do you think?

(Depends on PR #4)

You can merge this Pull Request by running git pull https://github.com/feross/filestream opt-disable-meta Or view, comment on, or merge it at:   https://github.com/DamonOehlman/filestream/pull/5

Commit Summary pass options through to Readable constructor add opts.meta option to emit metadata in stream don't assume opts will passed in File Changes

M
package.json
(3)

M
read.js
(14)

Patch Links: https://github.com/DamonOehlman/filestream/pull/5.patch https://github.com/DamonOehlman/filestream/pull/5.diff —Reply to this email directly or view it on GitHub.

feross commented 10 years ago

Okay, sounds good. Enjoy your holiday!

On Monday, July 7, 2014, Damon Oehlman notifications@github.com wrote:

Hey mate, I'll merge all your PRs through soon. Just on holidays at the moment :) From: Feross AboukhadijehSent: Tuesday, 8 July 2014 15:27To: DamonOehlman/filestreamReply To: DamonOehlman/filestreamSubject: [filestream] add 'meta' option to emit metadata in stream (#5)This is a great module! Thanks for making it.

One thing that tripped me when I was getting started with this module is that it emits the file's metadata into the stream. I think this is the wrong default behavior because it violates the 'principle of least astonishment'.

This PR turns off this behavior by default, and adds a meta option to re-enable it.

What do you think?

(Depends on PR #4)

You can merge this Pull Request by running git pull https://github.com/feross/filestream opt-disable-meta Or view, comment on, or merge it at: https://github.com/DamonOehlman/filestream/pull/5

Commit Summary pass options through to Readable constructor add opts.meta option to emit metadata in stream don't assume opts will passed in File Changes

M package.json (3)

M read.js (14)

Patch Links: https://github.com/DamonOehlman/filestream/pull/5.patch https://github.com/DamonOehlman/filestream/pull/5.diff —Reply to this email directly or view it on GitHub.

— Reply to this email directly or view it on GitHub https://github.com/DamonOehlman/filestream/pull/5#issuecomment-48274006.

Feross | blog http://feross.org/ | webtorrent http://webtorrent.io/ | studynotes http://www.apstudynotes.org/

DamonOehlman commented 10 years ago

Hey @feross - I've merged those two requests and will publish a new version soon. But I'll take a look at #2 before doing that also.

Thanks for the updates, it really is appreciated!