fix(#368): added utility function for elapsed time tracking #389
+146
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@maxonfjvipon please, check this one (ignore the build problems, they are not related to this branch) |
maxonfjvipon
suggested changes
Nov 23, 2024
src/commands/java/compile.js
Outdated
| @@ -33,6 +33,10 @@ const path = require('path'); | |||
| */ | |||
| module.exports = function(opts) { | |||
| const target = path.resolve(opts.target); | |||
| /** | |||
| * @todo #368 | |||
There was a problem hiding this comment.
@trapvpack it's a wrong format for todo, should be:
@todo #368:30min Description of todo...
more description with space at the beginning
src/elapsed.js
Outdated
|
|
||
| /** | ||
| * @todo #368 | ||
| * Consider if this method belong is in the right place. |
There was a problem hiding this comment.
@trapvpack see above about todo format
src/elapsed.js
Outdated
| } else { | ||
| extended = `${Math.ceil(duration / 3600000)}min`; | ||
| } | ||
| let msg = `${message} in ${extended}`; |
There was a problem hiding this comment.
@trapvpack I think msg can be const here
src/elapsed.js
Outdated
| */ | ||
| module.exports.elapsed = function elapsed(task) { | ||
| const startTime = Date.now(); | ||
| const tracked = { |
There was a problem hiding this comment.
@trapvpack let's inline this variable into task call
return task({
print: (message) => {
// code here
}
})
test/test_elapsed.js
Outdated
| const assert = require("assert"); | ||
|
|
||
| describe('elapsed', function(){ | ||
| const snooze = ms => new Promise(resolve => setTimeout(resolve, ms)); |
There was a problem hiding this comment.
@trapvpack let's follow a single code style and wrap lambda arguments with braces:
const snooze = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
test/test_elapsed.js
Outdated
| ) | ||
| ) | ||
| }); | ||
|
|
There was a problem hiding this comment.
@trapvpack there's no need for space here
test/test_elapsed.js
Outdated
| ) | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
@trapvpack there's no need for space here
test/test_elapsed.js
Outdated
| ) | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
@trapvpack there's no need for space here
test/test_elapsed.js
Outdated
| }); | ||
|
|
||
| it('handles errors in task correctly', async () => { | ||
| try { |
There was a problem hiding this comment.
@trapvpack I think the test can be simplified with assert.throws()
src/commands/assemble.js
Outdated
| }); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
@trapvpack this change is unnecessary
|
@maxonfjvipon take a look, I fixed what you asked for |
|
@trapvpack please, merge |
|
@trapvpack you didn't fix all the my comments and there are some new ones from |
|
@maxonfjvipon, take a look, I think I closed all conversations. If I understood correctly, it was necessary to modify conversations without the outdated tag, correct me if this is wrong |
|
@trapvpack all builds are clean in the |
|
@yegor256 @maxonfjvipon, take a look please |
|
@yegor256 @maxonfjvipon, take a look please😎 |
maxonfjvipon
suggested changes
Nov 26, 2024
src/commands/java/compile.js
Outdated
| @@ -33,6 +33,10 @@ const path = require('path'); | |||
| */ | |||
| module.exports = function(opts) { | |||
| const target = path.resolve(opts.target); | |||
| /** | |||
| * @todo #368:30min Wrap logs in 'elapsed' | |||
| * - It is necessary to use 'elapsed' in all logging cases that require output of elapsed time | |||
There was a problem hiding this comment.
@trapvpack the format is still wrong, should be:
* @todo #368:30min Wrap logs in 'elapsed'
* It is necessary to use 'elapsed' in all logging cases that require output of elapsed time
test/test_elapsed.js
Outdated
| await snooze(300); | ||
| return tracked.print('task'); | ||
| }) .then( | ||
| (actual)=> assert( |
There was a problem hiding this comment.
@trapvpack the space after (actual) is missed
test/test_elapsed.js
Outdated
| return elapsed(async (tracked) => { | ||
| await snooze(10); | ||
| return tracked.print('short task'); | ||
| }) .then( |
There was a problem hiding this comment.
@trapvpack the space after }) is unnecessary
test/test_elapsed.js
Outdated
| return elapsed(async (tracked) => { | ||
| await snooze(1200); | ||
| return tracked.print('long task'); | ||
| }) .then( |
There was a problem hiding this comment.
@trapvpack the space after }) is unnecessary
# Conflicts: # src/commands/java/compile.js # src/elapsed.js # test/test_elapsed.js
|
@maxonfjvipon, take a look please, thanks for your patience) |
maxonfjvipon
suggested changes
Nov 26, 2024
|
@maxonfjvipon, take a look please, I hope I understand correctly now |
maxonfjvipon
suggested changes
Nov 26, 2024
src/elapsed.js
Outdated
| * @todo #368:30min Decide if the `elapsed` utility function is in the right place, | ||
| * consider relocating it and its test file if needed | ||
| */ | ||
|
|
There was a problem hiding this comment.
@trapvpack you have two comments section here stated with /** and ended with */. The first comment is on lines 25-28, the second is on 30-44 and there's an empty line 29 between them. We don't need two comments here, it should one single comment started on line 25 and ended on line 43 or 44 with no empty lines. Also please move your todo under the function description
|
@maxonfjvipon, take a look please, thanks for the detailed explanation |
maxonfjvipon
suggested changes
Nov 27, 2024
src/elapsed.js
Outdated
| }); | ||
| }; | ||
| /** | ||
| * @todo #368:30min Decide if the `elapsed` utility function is in the right place, |
There was a problem hiding this comment.
@trapvpack now you have 2 comments again... it should be one:
/**
* A utility function to measure the elapsed time of a task and provide
* detailed timing information.
*
* This function wraps a given task (callback function) and provides it with
* a `tracked` object that includes a `print` method. The `print` method can
* be used within the task to log messages along with the elapsed time
* since the task started execution. The elapsed time is formatted in milliseconds,
* seconds, or minutes, based on the duration.
*
* @param {Function} task - A callback function to be measured. The function
* is invoked with a `tracked` object as an argument.
* @return {*} Result of the wrapped callback function. The result of the
* `task` callback will be returned unchanged.
* @todo #368:30min Decide if the `elapsed` utility function is in the right place,
* consider relocating it and its test file if needed
*/
|
@maxonfjvipon, take a look please |
maxonfjvipon
approved these changes
Nov 27, 2024
|
@yegor256, take a look please |
|
@yegor256, take a look please:), thanks for attention |
|
@rultor merge |
@trapvpack @yegor256 Oops, I failed. You can see the full log here (spent 21min). |
|
@rultor merge |
@trapvpack @yegor256 Oops, I failed. You can see the full log here (spent 22min). |
|
@rultor merge |
@trapvpack @yegor256 Oops, I failed. You can see the full log here (spent 19min). |
|
@trapvpack @maxonfjvipon something is wrong here? Why it doesn't build after merge? |
|
@yegor256 it seems something wrong with |
|
@maxonfjvipon is there anything I can do to help? |
|
@trapvpack let's wait for objectionary/eo2js#121, I'll fix it as quick as possible |
|
@trapvpack I released new version of |
|
@rultor merge |
@trapvpack @yegor256 Oops, I failed. You can see the full log here (spent 22min). |
|
@trapvpack thanks!! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #368
@yegor256 take a look please