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

Build counter progress #65

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
27 changes: 20 additions & 7 deletions packages/cli/src/command-build.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,20 @@ async function executeDuringPhase(
biltin,
dryRun,
) {
return await buildPackages(
finalPackagesToBuild,
makePackageBuild(jobConfiguration, rootDirectory, buildOptions, biltin),
dryRun,
)
/**@return {import('@bilt/build').BuildPackageFunction} */
const buildCounter = () => {
Copy link
Owner

Choose a reason for hiding this comment

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

If pointer does not need to be passed (see my other comment below), then there's no real need for this function.

Copy link
Author

Choose a reason for hiding this comment

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

the pointer is passing down - I use closer to increment it

let pointer = 1
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of this variable?

Copy link
Author

Choose a reason for hiding this comment

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

this is the variable in the closer - for increment it - do you have another suggestion?

return makePackageBuild(
jobConfiguration,
rootDirectory,
buildOptions,
biltin,
Object.keys(finalPackagesToBuild).length,
pointer,
)
}

return await buildPackages(finalPackagesToBuild, buildCounter(), dryRun)
}

/**
Expand Down Expand Up @@ -274,6 +283,7 @@ async function buildPackages(
built: /**@type {Package[]}*/ ([]),
}
debug('starting build')

for await (const buildPackageResult of build(packageInfosToBuild, buildOrder, buildPackageFunc)) {
debug(
`build of ${buildPackageResult.package.directory} ended. result: ${
Expand Down Expand Up @@ -479,6 +489,8 @@ function makePackageBuild(
/**@type {import('@bilt/types').Directory}*/ rootDirectory,
/**@type {{[x: string]: string|boolean | undefined}} */ buildOptions,
/**@type {object} */ biltin,
/**@type {(number)} */ packagesLength,
/**@type {(number)} */ pointer,
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 see why this needs to be a parameter. It can be a local variable

Copy link
Author

@yanai101 yanai101 May 20, 2021

Choose a reason for hiding this comment

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

changing the function

) {
/**@type import('@bilt/build').BuildPackageFunction */
return async function ({packageInfo}) {
Expand All @@ -487,11 +499,12 @@ function makePackageBuild(
packageInfo.directory,
))

packageHeader('building', packageInfo)
packageHeader('building', packageInfo, pointer, packagesLength)
Copy link
Owner

Choose a reason for hiding this comment

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

packageIndex would be a better name than pointer. Or maybe buildIndex?

Copy link
Author

Choose a reason for hiding this comment

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

yap - changing


await executePhase(jobConfiguration, 'during', packageDirectory, buildOptions, biltin, (se) =>
packageOperation(se.info().name, packageInfo),
)

++pointer
return 'success'
}
}
Expand Down
9 changes: 7 additions & 2 deletions packages/cli/src/outputting.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,14 @@ export function globalOperation(msg) {
/**
* @param {string} msg
* @param {import('@bilt/types').PackageInfo} packageInfo
* @param {number | null} current
Copy link
Owner

Choose a reason for hiding this comment

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

I usually prefer not putting null in my code. undefined is the only one I use.

Copy link
Author

Choose a reason for hiding this comment

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

ok

* @param {number | null} length
*/
export function packageHeader(msg, packageInfo) {
outputFunction(chalk.greenBright.underline(`**** [${packageInfo.directory}] ${msg}`))
export function packageHeader(msg, packageInfo, current = null, length = null) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why would a null/undefined be passed here? All calls to packageHeader will probably pass both current and length.

const counterMessage = current && length ? `${current} of ${length}` : ''
outputFunction(
chalk.greenBright.underline(`**** [${packageInfo.directory}] ${msg} ${counterMessage} `),
Copy link
Owner

Choose a reason for hiding this comment

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

(Could we make the counter message shorter? And in parenthesis? E.g. (#1/10)

Copy link
Author

Choose a reason for hiding this comment

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

sure!

)
}

/**
Expand Down
Loading