Add prefs changetimeout (issue #99)#122
Conversation
|
@prasadtalasila Sir I haven't written the tests for this yet, however I was able to run it properly and it seemed to work fine (for now)...The cli-prefs.json file is also getting properly updated |
|
Also I've used 'timeouttype' instead of 'type' because type is already being used to denote the server type |
|
@mukkachaitanya please review this PR. |
mukkachaitanya
left a comment
mukkachaitanya
left a comment
There was a problem hiding this comment.
The PR looks great. However, there are some minor changes and comment. Please have a look at them @rrgodhorus and sir, @prasadtalasila.
| const { type, time } = changeTimeoutEvent.details; | ||
| const supportedTimeoutTypes = Object.keys(defaultPrefs.timeouts); | ||
| if (!supportedTimeoutTypes.includes(type)) { | ||
| console.log('DEbug'); |
There was a problem hiding this comment.
Please remove this debug log.
|
|
||
| const validateChangeTimeout = (changeTimeoutEvent) => { | ||
| const { type, time } = changeTimeoutEvent.details; | ||
| const supportedTimeoutTypes = Object.keys(defaultPrefs.timeouts); |
There was a problem hiding this comment.
Please use lodash 's function .keys() to have uniformity across the project for such functionality.
| }, | ||
| }; | ||
| } | ||
| if (!time || !validator.isInt(time) || !(parseInt(time, 10) > 0)) { |
There was a problem hiding this comment.
A good check for the timeout to be positive 👍
There was a problem hiding this comment.
Also maybe you can consider using the options field of the validator. So the whole statement reduces to
if(!time || !validator.isInt(time, { gt: '0' }){
...
}| 'cliPrefs', | ||
| { | ||
| timeouts: { | ||
| session: +time, |
There was a problem hiding this comment.
Okay,I'll change it to parseInt() then ?
There was a problem hiding this comment.
I used + because it is used in line 65, maxSize: +details.maxSize
| // const SECONDS = 60; | ||
| // const MINUTES = 60; | ||
| // const HOURS = 2; | ||
|
|
There was a problem hiding this comment.
These comments can be removed.
| logger_dir: cliPrefs.logger.logDirectory, | ||
| logger_location: cliPrefs.logger.logLocation, | ||
| logger_blacklist: cliPrefs.logger.blacklist, | ||
| timeouts_session: cliPrefs.timeouts.session, |
There was a problem hiding this comment.
We might need to decouple the model and output for prefs. What do you think, sir @prasadtalasila?
Well, it's not of concern with regard to this PR, can go in another one later.
| .option('--lang <lang>', 'Change language') | ||
| .option('--maxsize <size>', 'Change logger file maxsize') | ||
| .option('--blacklist <keyword>', 'Add keyword to logger blacklist') | ||
| .option('--timeouttype <type>', 'Timeout type ( session ) ') |
There was a problem hiding this comment.
An option of the form --timeout-type would be preferred. Caporal does support such options, see here
Codecov Report
@@ Coverage Diff @@
## dev #122 +/- ##
==========================================
- Coverage 99.28% 91.95% -7.33%
==========================================
Files 24 24
Lines 695 746 +51
==========================================
- Hits 690 686 -4
- Misses 5 60 +55
Continue to review full report at Codecov.
|
3 participants
Requirements
Description of the Change
Added 'changetimeout' pref which currently accepts session as timeout type and time in seconds.
Benefits
Session timeout is no longer a hard-coded value
Possible Drawbacks
Applicable Issues