Updates after refactoring Connection constructor #45

abmusse · 2019-03-25T21:38:16Z

Changes to Toolkit.js

After refactoring Connection construct options were added to return an error on the user provided callback to Connection.run(). Toolkit.js was previously updated with a632dbf. These changes do not take into account when returnError is set to false. returnError is to false when iConn class is used. This is because in pre v.1.0 version errors were not returned. In that case run will only return XML output instead (error, output).

Now with 4236059: added in checks for when returnError is false Toolkit.js. Within the constructor a new Connection based on the transport options from the conn parameter is created when returnError is false. This was done to avoid worrying about setting it back to on at some point and possible race conditions if the same connection object is used elsewhere.

Added CommandCall Class

CommandCall class was added to to maintain command types (sh, cl, qsh). Please view this issue for reasoning.

Updated Deprecated.js

Now depd package to display deprecation warnings
wrap getConnection() to call getTransportOptions
have setTimeout() print warningn no longer used

Updated unit/functional tests

Both unit and functional tests updated to use current non deprecated
versions.

Moved existing unit/functional tests:

Functional tests using deprecated class moved to test/functional/deprecated
Unit tests using deprecated classes moved to test/unit/deprecated

Added ToolkitFunctional.js

remove functional test deprecated files (iDataQueueFunctional, ...)
consolidates all Toolkit methods into ToolkitFunctional.js

Add in checks for when returnError is false in Toolkit.js. This is case occurs when a call from deprecated class goes through Toolkit.js. Instead of worrying about setting the flag on and off create a new Connection object when same config when returnError is false within the constructor. Then have proper checks for when to report the error.

kadler · 2019-03-25T21:50:08Z

Seems really weird to have a single CommandCall object which takes in varying configuration options and then having specific methods on it for the various types (ie. qsh, cl, sh). Why not just have three separate classes or have a single class which takes in a parameter for the type of command to call?

markdirish · 2019-03-25T22:29:18Z

Probably better to have three separate classes I would think. I think I combined them into CommandCall when I had the zeal of cutting down as much of the duplicated code in this project as possible.

abmusse · 2019-03-25T23:58:47Z

@kadler

Why not have a single class which takes in a parameter for the type of command to call

The current constructor for CommandCall accepts an object with the format:

{
 command: ' '
 type: 'sh' || 'cl' || 'qsh',
 [options]: {}
}

command and type properties are required.

So CommandCall does have a parameter for the type of command to call.

In toXML the appropriate method (sh, qsh, cl) based on the type specified is called.

Ideally, I would like to have sh, qsh, and cl methods to be private to the class but I don't think that's possible in JS yet.

I would like to have had toXML from CommandCall to be the only public accessible method.

abmusse · 2019-03-26T00:00:10Z

Why not just have three separate classes?

Having 3 separate classes works too.

The reason I did not go that route first was to keep things less verbose.

If we went that route the classes themselves would be pretty thin for now.

Maybe something like:

class CLCommand {

    constructor(command, options) {
      if (!command || typeof command !== 'string') {
	   throw new Error('Command is required');
      }

      this.command = command;
      
      if (options && typeof options === 'object') {
         this.options = options;
      }
      
    }


    toXML() {
       .... code to generate and return xml
   }
}

The other 2 command classes would be similar

kadler · 2019-03-26T15:23:20Z

In toXML the appropriate method (sh, qsh, cl) based on the type specified is called.

I see now. In that case, I'd just merge those functions in to toXML

kadler · 2019-03-26T15:25:04Z

I really hate this iXmlNodeCmdOpen stuff as well. It's all overly complicated and over-engineered.

abmusse · 2019-03-26T16:04:50Z

I really hate this iXmlNodeCmdOpen stuff as well. It's all overly complicated and over-engineered.

We should consider refactoring and simplifying ixml.js in a future PR.

kadler · 2019-03-26T16:19:50Z

We should consider refactoring and simplifying ixml.js in a future PR.

Agreed. I opened #46 to keep track of it.

lib/CommandCall.js

abmusse · 2019-03-26T21:37:18Z

Merged (cl, sh, qsh) functions into toXML().

Reverted back to use cl instead of cmd.

lib/CommandCall.js

- class to maintain command types (sh, cl, qsh) see issue #24 - merge (sh, cl, qsh) code into toXml - add in some JSDoc annotations

- add CommandCall to exports - remove previous generateXml functions

- Use depd package to display deprecation warnings iConn Class - wrap getConnection() to call getTransportOptions - have setTimeout() print warning Use route (iSh, iCmd, iQsh) to use CommandCall

update returnTransports function - use single object to configure the Connection - add transportOptions to options in returnTransport add returnTrasportsDeprecated - transports using iConn used by depercated functional tests

Functional tests using deprecated class moved to: - test/functional/deprecated Unit tests using deprecated classes moved to: - test/unit/deprecated This was done to keep pre v1.0 tests around to ensure compatability. At some point we will stop supporting these in the future. Then we can simply git rm these two folders

Both unit and functional tests updated to use current non deprecated versions Unit Tests iConnUnit.js - remove setTimeout() test because method no longer exists Functional Tests - update run function callback to include error first - rename user -> username for options object

- consolidates all Toolkit methods into one test file - remove deprecated files

- iPgmFunctional -> ProgramCallFunctional - iSqlFunctional -> SqlCallFunctional - commandsFunctional -> CommandCallFunctional - iConnUnit -> ConnectionUnit - iPgmUnit -> ProgramCallUnit - iSqlUnit -> SqlCallunit

abmusse · 2019-03-27T22:51:32Z

I will squash e33cc8c and e164691 with d2e8b57

abmusse requested review from kadler, dmabupt and markdirish March 25, 2019 21:38

kadler reviewed Mar 26, 2019

View reviewed changes

lib/CommandCall.js Outdated Show resolved Hide resolved

kadler reviewed Mar 27, 2019

View reviewed changes

lib/CommandCall.js Outdated Show resolved Hide resolved

kadler approved these changes Mar 27, 2019

View reviewed changes

abmusse added 10 commits March 27, 2019 15:48

Add CommandCall Class

d9814f4

- class to maintain command types (sh, cl, qsh) see issue #24 - merge (sh, cl, qsh) code into toXml - add in some JSDoc annotations

Rename command.js -> CommandCall.js

ca7b3d9

Update itoolkit.js

9827739

- add CommandCall to exports - remove previous generateXml functions

Update Deprecated.js

8aa5492

- Use depd package to display deprecation warnings iConn Class - wrap getConnection() to call getTransportOptions - have setTimeout() print warning Use route (iSh, iCmd, iQsh) to use CommandCall

Update utils.js

6a48a04

update returnTransports function - use single object to configure the Connection - add transportOptions to options in returnTransport add returnTrasportsDeprecated - transports using iConn used by depercated functional tests

Added ToolkitFunctional.js

abbdfe4

- consolidates all Toolkit methods into one test file - remove deprecated files

Rename unit/functional tests

55deff6

- iPgmFunctional -> ProgramCallFunctional - iSqlFunctional -> SqlCallFunctional - commandsFunctional -> CommandCallFunctional - iConnUnit -> ConnectionUnit - iPgmUnit -> ProgramCallUnit - iSqlUnit -> SqlCallunit

Bump default timeout for tests from 2s -> 3s

b8ffc5b

abmusse merged commit 6ef2cb3 into IBM:v1.0-dev Mar 28, 2019

abmusse deleted the updates branch May 8, 2020 02:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates after refactoring Connection constructor #45

Updates after refactoring Connection constructor #45

abmusse commented Mar 25, 2019

kadler commented Mar 25, 2019

markdirish commented Mar 25, 2019

abmusse commented Mar 25, 2019 •

edited

Loading

abmusse commented Mar 26, 2019 •

edited

Loading

kadler commented Mar 26, 2019

kadler commented Mar 26, 2019

abmusse commented Mar 26, 2019

kadler commented Mar 26, 2019 •

edited

Loading

abmusse commented Mar 26, 2019 •

edited

Loading

abmusse commented Mar 27, 2019

Updates after refactoring Connection constructor #45

Updates after refactoring Connection constructor #45

Conversation

abmusse commented Mar 25, 2019

Changes to Toolkit.js

Added CommandCall Class

Updated Deprecated.js

Updated unit/functional tests

kadler commented Mar 25, 2019

markdirish commented Mar 25, 2019

abmusse commented Mar 25, 2019 • edited Loading

abmusse commented Mar 26, 2019 • edited Loading

kadler commented Mar 26, 2019

kadler commented Mar 26, 2019

abmusse commented Mar 26, 2019

kadler commented Mar 26, 2019 • edited Loading

abmusse commented Mar 26, 2019 • edited Loading

abmusse commented Mar 27, 2019

abmusse commented Mar 25, 2019 •

edited

Loading

abmusse commented Mar 26, 2019 •

edited

Loading

kadler commented Mar 26, 2019 •

edited

Loading

abmusse commented Mar 26, 2019 •

edited

Loading