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

add number of reversions option #3

Closed
wants to merge 2 commits into from

Conversation

stevemao
Copy link
Contributor

Ref: sindresorhus/vinyl-paths#2
Only thing is its probably not the best use-case to demonstrate in the README.md. However I can't use double dest because the base would change in after the first dest(). gulpjs/vinyl-fs#77

@stevemao stevemao force-pushed the number-of-reversions branch from 26a3bbd to 7dadbdb Compare July 11, 2015 02:59
return through.obj(function (file, enc, cb) {
var history = file.history;

if (history.length > 1) {
if (history.length > reversions) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think it should just silently not do its thing when there aren't enough history. Either throw or reverse as much as possible. Not sure which.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverse as much as possible? Since it's backward compatible any no one raised a request on throwing an error before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I think it makes more sense now

Copy link
Owner

Choose a reason for hiding this comment

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

reverse as much as possible?

👍

@stevemao stevemao force-pushed the number-of-reversions branch from 7dadbdb to 0189276 Compare July 13, 2015 23:58
@stevemao
Copy link
Contributor Author

gulpjs/vinyl-fs#77 is merged and the added tests gulpjs/vinyl-fs#79 passed. We could use double dest in the example (which makes more sense than the current one.).

@sindresorhus
Copy link
Owner

We could use double dest in the example (which makes more sense than the current one.).

👍

@@ -1,15 +1,19 @@
'use strict';
var through = require('through2');

module.exports = function () {
module.exports = function (reversions) {
reversions = reversions || 1;
Copy link
Owner

Choose a reason for hiding this comment

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

reversions = typeof reversions === 'number' ? reversions : 1; as users might want to explicitly pass in 0 for some reason.

Copy link
Owner

Choose a reason for hiding this comment

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

should also have an empty line below this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very true

@stevemao stevemao force-pushed the number-of-reversions branch from 04f7513 to 295e33a Compare July 28, 2015 04:26
@stevemao
Copy link
Contributor Author

double dest example needs to wait until vinyl-fs is released.

@sindresorhus
Copy link
Owner

Looks good. Thanks :)

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