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

Make release tools asynchronous #878

Merged
merged 12 commits into from
May 16, 2023
Merged

Conversation

pomek
Copy link
Member

@pomek pomek commented May 15, 2023

Suggested merge commit message (convention)

Internal: Make release tools asynchronous. Closes ckeditor/ckeditor5#14135.

@pomek pomek changed the title Changed updateDependencies() to be async. Make release tools asynchronous May 15, 2023
@coveralls
Copy link

coveralls commented May 15, 2023

Coverage Status

Coverage: 88.857% (+0.01%) from 88.846% when pulling 7341cf3 on ck/14135-async into 1b26a06 on ck/epic/13950-release-process.

@przemyslaw-zan przemyslaw-zan marked this pull request as ready for review May 15, 2023 16:05
@pomek pomek force-pushed the ck/14135-async branch from 50ee416 to 5e5b5ca Compare May 15, 2023 18:48
@pomek pomek requested a review from psmyrek May 15, 2023 19:12
Copy link
Contributor

@psmyrek psmyrek left a comment

Choose a reason for hiding this comment

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

LGTM 🎉


const counter = tools.createSpinner( processDescription, { total: packages.length } );
counter.start();
const onPackageDone = progressFactory( listrTask, packages.length );
Copy link
Contributor

@martnpaneq martnpaneq May 16, 2023

Choose a reason for hiding this comment

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

Looks like progressFactory could be passed as an argument to executeInParallel. This way executeInParallel would be independent of the task runner if we chose to use a different one in the future for other use cases.

The api could look somewhat like this

executeInParallel( {
	packagesDirectory: 'packages',
	progressFactory,
	taskToExecute: () => {}
} );

function progressFactory( totalPackages ) {
	let done = 0;

	return () => {
		done++;
		listrTask.output = 'task finished. Total: ' + done + '/' + totalPackages;
	};
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but it's hard to predict what arguments it should receive. I would keep it in mind in the future to refactor it.

@pomek pomek merged commit d53e284 into ck/epic/13950-release-process May 16, 2023
@pomek pomek deleted the ck/14135-async branch May 16, 2023 12:46
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.

5 participants