Skip to content

Conversation

@liuhanqu
Copy link
Contributor

deletedCount should start from 1

@sindresorhus
Copy link
Owner

// @jopemachine

Copy link
Contributor

@jopemachine jopemachine left a comment

Choose a reason for hiding this comment

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

Thanks for correcting this :)
It was my mistake when occurred in changing the call order of onProgress.

The below function call should be removed since the last element will be called twice by the line.

del/index.js

Lines 99 to 103 in 7fbeca0

onProgress({
totalCount: files.length,
deletedCount: files.length,
percent: 1
});

Copy link
Contributor

@jopemachine jopemachine left a comment

Choose a reason for hiding this comment

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

And I think using fileIndex through mapper's argument here was another mistake.
deletedCount should be in ascending order semantically, but because the mapper function is async, it will be called in order mapper's promise resolve.

It means the below test will break.

del/test.js

Line 381 in 7fbeca0

test('onProgress option - progress of multiple files', async t => {

I think probably this should be fixed with like below codes.

let fileIndex = 1;
const mapper = async (file) => {
// ...
	onProgress({
			totalCount: files.length,
			deletedCount: fileIndex,
			percent: fileIndex / files.length
	});
	++fileIndex;
// ...
}

@liuhanqu
Copy link
Contributor Author

let count  = 0
const mapper = async (file, fileIndex) => {
    file = path.resolve(cwd, file);

    if (!force) {
	safeCheck(file, cwd);
    }

    if (!dryRun) {
	await rimrafP(file, rimrafOptions);
    }

    count += 1

    onProgress({
	  totalCount: files.length,
	  deletedCount: count,
	  percent: count / files.length
    });

    return file;
};

Agree your point, but i think we should replace fileIndex with count while it's more semantic.

@jopemachine
Copy link
Contributor

jopemachine commented May 17, 2022

Agree your point, but i think we should replace fileIndex with count while it's more semantic.

I agree the variable needs be renamed, but I think deletedCount would be better. (and fileIndex should be removed.)

@liuhanqu
Copy link
Contributor Author

In the doc, the onProgress arguments is not the same in the code.

{
	totalFiles: number,
	deletedFiles: number,
	percent: number
}

xxxFiles or xxxCount, which one ?

@jopemachine
Copy link
Contributor

Thanks for correcting.

That should be xxxCount.

@jopemachine
Copy link
Contributor

@sindresorhus
I'm sorry for the mess.
I will be more careful in handling feedback.

@sindresorhus sindresorhus changed the title Fix deletedCount Fix ProgressData#deletedCount May 24, 2022
@sindresorhus sindresorhus merged commit 7b4c881 into sindresorhus:main May 24, 2022
@sindresorhus
Copy link
Owner

Thanks, everyone :)

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.

3 participants