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 missing space in log output #738

Closed
damithc opened this issue Mar 2, 2019 · 3 comments · Fixed by #748
Closed

Fix missing space in log output #738

damithc opened this issue Mar 2, 2019 · 3 comments · Fixed by #748

Comments

@damithc
Copy link
Contributor

damithc commented Mar 2, 2019

Current:
image

A space is missing? This can be phrased in a more readable way? e.g., 1234 ms

@tawAsh1
Copy link
Contributor

tawAsh1 commented Mar 2, 2019

Hello.

Is this output necessary?
As far as I saw the code here, it seems that the site creation time has already been output in here "Website generation complete! Total build time: 6.126s".

markbind/src/Site.js

Lines 506 to 536 in ba77643

Site.prototype.generate = function (baseUrl) {
const startTime = new Date();
// Create the .tmp folder for storing intermediate results.
logger.profile(GENERATE_SITE_LOGGING_KEY);
fs.emptydirSync(this.tempPath);
// Clean the output folder; create it if not exist.
fs.emptydirSync(this.outputPath);
logger.info(`Website generation started at ${startTime.toLocaleTimeString()}`);
return new Promise((resolve, reject) => {
this.readSiteConfig(baseUrl)
.then(() => this.collectAddressablePages())
.then(() => this.collectBaseUrl())
.then(() => this.collectUserDefinedVariablesMap())
.then(() => this.collectPlugins())
.then(() => this.buildAssets())
.then(() => this.buildSourceFiles())
.then(() => this.copyMarkBindAsset())
.then(() => this.copyLayouts())
.then(() => this.updateSiteData())
.then(() => {
const endTime = new Date();
const totalBuildTime = (endTime - startTime) / 1000;
logger.info(`Website generation complete! Total build time: ${totalBuildTime}s`);
})
.then(resolve)
.catch((error) => {
rejectHandler(reject, error, [this.tempPath, this.outputPath]);
})
.finally(() => logger.profile(GENERATE_SITE_LOGGING_KEY));
});
};

@damithc
Copy link
Contributor Author

damithc commented Mar 2, 2019

Is this output necessary?

Good question. The two numbers seem to differ slightly though. Let's wait to hear from a senior developer.

@yamgent
Copy link
Member

yamgent commented Mar 2, 2019

Yes, this line popped up because we now print everything to the console since #721 came. Previously it was hidden inside the log, and never printed on the console.

The duration given by the profiler is always printed regardless of whether the site builds successfully or not (which is why it takes a slightly longer time before printing, compared to the one that prints straight away immediately after the generation is complete).

I don't think it is useful to print the duration when the site fails to build, so it should not be shown to the authors. Instead, we should explore downgrading the log levels for profiler messages, so that it doesn't show up on "info:" level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants