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

Reverse order back for the returned paths #99

Merged

Conversation

chrisblossom
Copy link
Contributor

#95 might be considered a breaking change since the files are sorted in reverse order. I am not sure this reverts the old order exactly, but thought I'd put this PR in to let you decide what's best.

@sindresorhus
Copy link
Owner

Why do you think it’s a breaking change?

@chrisblossom
Copy link
Contributor Author

Currently if someone has code like this:

test('removes files', async () => {
    // assume these files exist
    // './remove-these-files/a.js'
    // './remove-these-files/b.js'
    const filesRemoved = await del(['**/*'], {
        cwd: path.resolve('./remove-these-files'),
    });

    expect(filesRemoved).toEqual([
        path.resolve('./remove-these-files/a.js'),
        path.resolve('./remove-these-files/b.js'),
    ]);
});

Their test will fail after this update.

Not 100% saying it is breaking, just something to consider. I noticed it on one of my projects.

@sindresorhus
Copy link
Owner

sindresorhus commented Jul 13, 2019

The order was already unstable as globbing is inherently not stable, and the order could change in patch releases because of glob optimizations. So yes, makes sense to sort it back for consistency.

@sindresorhus sindresorhus changed the title Reverse order back to prevent breaking change Reverse order back for the returned paths Jul 13, 2019
@sindresorhus sindresorhus merged commit 51662ac into sindresorhus:master Jul 13, 2019
@chrisblossom chrisblossom deleted the re-sort-after-completion branch July 13, 2019 08:03
@chrisblossom
Copy link
Contributor Author

The order was already unstable as globbing is inherently not stable, and the order could change in patch releases because of glob optimizations. So yes, makes sense to sort it back for consistency.

After thinking about this comment I think this PR should be updated to use .sort((a, b) => a.localeCompare(b)) instead of .reverse() to consistently return the same order every time.

Should I put another PR in for this, or do you want to amend this merge/leave as is?

@sindresorhus
Copy link
Owner

Sure, PR welcome.

@chrisblossom chrisblossom mentioned this pull request Jul 15, 2019
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.

None yet

2 participants