-
Notifications
You must be signed in to change notification settings - Fork 23
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
base: main
Are you sure you want to change the base?
Conversation
@giltayar can you check this please. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Some small comments, plus I'm missing a test or two that checks this functionality...
packages/cli/src/command-build.js
Outdated
) | ||
/**@return {import('@bilt/build').BuildPackageFunction} */ | ||
const buildCounter = () => { | ||
let pointer = 1 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
packages/cli/src/command-build.js
Outdated
@@ -487,11 +499,12 @@ function makePackageBuild( | |||
packageInfo.directory, | |||
)) | |||
|
|||
packageHeader('building', packageInfo) | |||
packageHeader('building', packageInfo, pointer, packagesLength) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yap - changing
packages/cli/src/outputting.js
Outdated
@@ -38,9 +38,14 @@ export function globalOperation(msg) { | |||
/** | |||
* @param {string} msg | |||
* @param {import('@bilt/types').PackageInfo} packageInfo | |||
* @param {number | null} current |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
packages/cli/src/outputting.js
Outdated
*/ | ||
export function packageHeader(msg, packageInfo) { | ||
outputFunction(chalk.greenBright.underline(`**** [${packageInfo.directory}] ${msg}`)) | ||
export function packageHeader(msg, packageInfo, current = null, length = null) { |
There was a problem hiding this comment.
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
.
export function packageHeader(msg, packageInfo, current = null, length = null) { | ||
const counterMessage = current && length ? `${current} of ${length}` : '' | ||
outputFunction( | ||
chalk.greenBright.underline(`**** [${packageInfo.directory}] ${msg} ${counterMessage} `), |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure!
packages/cli/src/command-build.js
Outdated
@@ -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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing the function
packages/cli/src/command-build.js
Outdated
dryRun, | ||
) | ||
/**@return {import('@bilt/build').BuildPackageFunction} */ | ||
const buildCounter = () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@yanai101, please merge main branch because I fixed a test in |
Adding to the header - # of # counter
This change is