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

Fix ProgressData#deletedCount #142

Merged
merged 2 commits into from
May 24, 2022
Merged

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