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.

Sign up for GitHub

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

Jump to bottom

feat(#93) add compilation status messages #98

Merged
merged 17 commits into from Dec 2, 2022
Merged

feat(#93) add compilation status messages #98

merged 17 commits into from Dec 2, 2022

Conversation

volodya-lombrozo
Copy link
Member

I've added some information about compilation process, now it looks like:

EO version is 0.28.11
[eo:register] 1808 ms. Total number of compiled files 0
EO objects registered in /Users/lombrozo/Workspace/EOlang/Projects/sum/.eoc/eo-foreign.json
EO version is 0.28.11
[eo:assemble] 33516 ms. Total number of compiled files 446
EO program assembled in /Users/lombrozo/Workspace/EOlang/Projects/sum/.eoc
EO version is 0.28.11
[eo:transpile] 11081 ms. Total number of compiled files 928
Java sources generated in /Users/lombrozo/Workspace/EOlang/Projects/sum/.eoc/generated-sources

It's important to notice that since spawn function is asynchronous and by that reason I have to wrap it and all calls using async-style chains (and Promise), it looks like:

register(program.opts())
.then((r) => assemble(program.opts()))
.then((r) => transpile(program.opts()))
.then((r) => compile(program.opts()))
.then((r) => link(program.opts()))
.then((r) => test(program.opts()));

Closes: #93

@volodya-lombrozo
Copy link
Member Author

@yegor256 pls, take a look

@volodya-lombrozo volodya-lombrozo marked this pull request as draft November 30, 2022 21:22
@volodya-lombrozo volodya-lombrozo marked this pull request as ready for review November 30, 2022 21:25
yegor256
yegor256 reviewed Dec 1, 2022
View reviewed changes
src/mvnw.js Outdated
]);
const cmd = bin + ' ' + params.join(' ');
console.debug('+ %s', cmd);
const result = spawn(
Copy link
Member

Choose a reason for hiding this comment

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

@volodya-lombrozo somewhere you indent by two spaces, somewhere by four. I'm surprised eslint doesn't catch you.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yegor256 fixed. I also have added new issue for that.

@volodya-lombrozo volodya-lombrozo mentioned this pull request Dec 1, 2022
Closed
@volodya-lombrozo
Copy link
Member Author

@yegor256 Please, check

yegor256
yegor256 reviewed Dec 1, 2022
View reviewed changes
src/commands/audit.js Outdated
@@ -31,5 +31,5 @@ const parserVersion = require('../parser-version');
*/
module.exports = function(opts) {
parserVersion();
mvnwSync(['--version']);
mvnw(['--version']);
Copy link
Member

@yegor256 yegor256 Dec 1, 2022 •
edited

Choose a reason for hiding this comment

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

@volodya-lombrozo why don't you use then here? Without it, if I understand correctly, the execution in this parallel "process" will still be running, while the function the audit will exist. Or I'm wrong? (I'm not an expert in JS)

Copy link
Member Author

Choose a reason for hiding this comment

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

@yegor256 We can use Promises without then and everything will works fine. Moreover, our tests prove that everything works fine. Anyway I've added the return statement for audit to give a caller the chance to call .then - perhaps it will be useful in the future.

yegor256
yegor256 reviewed Dec 1, 2022
View reviewed changes
src/status.js Outdated
@@ -0,0 +1,95 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

@volodya-lombrozo what's the point of making it a separate file/module? We use it only in mvnw.js? I think it will be more compact to put all this code in to mvnw.js

Copy link
Member Author

Choose a reason for hiding this comment

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

@yegor256 yes, it looks better now. Please check.

@volodya-lombrozo
Copy link
Member Author

@yegor256 have a look

@yegor256
Copy link
Member

yegor256 commented Dec 2, 2022

@rultor merge

@rultor
Copy link
Contributor

rultor commented Dec 2, 2022

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit 0a2af26 into objectionary:master Dec 2, 2022
@rultor
Copy link
Contributor

rultor commented Dec 2, 2022

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 9min)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yegor256 yegor256 Awaiting requested review from yegor256

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

use spawn() instead of spawnSync()
3 participants
@volodya-lombrozo @yegor256 @rultor