refactor: Remove ixml refs in CommandCall#103
refactor: Remove ixml refs in CommandCall#103abmusse merged 1 commit intoIBM:masterfrom abmusse:remove-ixml-for-commandCall
Conversation
|
Test |
lib/CommandCall.js
Outdated
| if (this.type === 'cmd' && !this.options.exec) { // set default value | ||
| // eslint-disable-next-line no-unused-expressions | ||
| this.command.indexOf('?') > 0 ? this.xml += " exec='rexx'" : this.xml += " exec='cmd'"; | ||
| } | ||
|
|
||
| if (!this.options.error) { // set default value | ||
| this.xml += " error='fast'"; | ||
| } |
There was a problem hiding this comment.
Similar to the other review, I would like to see these a little more DRY.
lib/CommandCall.js
Outdated
| if (this.type === 'cl') { | ||
| const defaultExec = (this.command.indexOf('?') > 0) ? 'rexx' : 'cmd'; | ||
| this.type = 'cmd'; | ||
| this.xml = `<${this.type} exec='${this.options.exec || defaultExec}'`; |
There was a problem hiding this comment.
| this.xml = `<${this.type} exec='${this.options.exec || defaultExec}'`; | |
| this.xml = `<cmd exec='${this.options.exec || defaultExec}'`; |
No reason to change this.type. That would actually cause problems if they re-use it, since it will go through the else code path and not check for rexx execution.
There was a problem hiding this comment.
lib/CommandCall.js
Outdated
| if (this.options.before) this.xml += ` before='${this.options.before}'`; | ||
| if (this.options.after) this.xml += ` after='${this.options.after}'`; | ||
|
|
||
| this.xml += ` error='${this.options.error || 'fast'}'>${this.command}</${this.type}>`; |
There was a problem hiding this comment.
We would also need to up this.type here as well
lib/CommandCall.js
Outdated
| if (this.options.before) this.xml += ` before='${this.options.before}'`; | ||
| if (this.options.after) this.xml += ` after='${this.options.after}'`; | ||
|
|
||
| const type = this.type === 'cl' ? 'cmd' : this.type; |
There was a problem hiding this comment.
Oh, right. I forgot we needed the type for the closing tag. I would suggest setting the local variable above then:
const type = this.type === 'cl' ? 'cmd' : this.type;
if(type == 'cmd') {
...
}
else {
...
}There was a problem hiding this comment.
Could even do:
const type = this.type === 'cl' ? 'cmd' : this.type;
this.xml = `<${this.type}`;
if (type == 'cmd') {
const defaultExec = (this.command.indexOf('?') > 0) ? 'rexx' : 'cmd';
this.xml += ` exec='${this.options.exec || defaultExec}'`;
}
markirish
left a comment
There was a problem hiding this comment.
LGTM
This looks so, so, so much cleaner and better. Especially love the availableCommands.join
Part of #96