Skip to content

refactor: ProgramCall.addParam to accept an object #139

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.

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

Merged
merged 11 commits into from
Mar 25, 2020
Merged

refactor: ProgramCall.addParam to accept an object #139

merged 11 commits into from
Mar 25, 2020

Conversation

abmusse
Copy link
Member

@abmusse abmusse commented Mar 11, 2020

Resolves #107

@abmusse abmusse changed the title refactor: Add functions to handle DS and simple data nodes refactor: ProgramCall.addParam to accept an object Mar 11, 2020
@abmusse abmusse requested a review from kadler March 11, 2020 18:05
@abmusse abmusse marked this pull request as ready for review March 11, 2020 18:05
@abmusse
Copy link
Member Author

abmusse commented Mar 12, 2020

There is an open proposal to add private methods to JS.

https://github.com/tc39/proposal-private-methods

It is currently a stage 3 candidate.

In the future we could use this syntax instead of using __mehtod__ naming used here.

class ClassWithPrivateMethod {
  #privateMethod() {
    return 'hello world'
  }

  getPrivateMessage() {
      return #privateMethod()
  }
}

@abmusse
Copy link
Member Author

abmusse commented Mar 12, 2020

Need to update unit ProgramCallUnit,js tests with this PR.

@kadler kadler closed this Mar 12, 2020
@kadler kadler reopened this Mar 12, 2020
@abmusse abmusse requested a review from kadler March 23, 2020 03:15
@abmusse
Copy link
Member Author

abmusse commented Mar 23, 2020

Functions who use ProgramCall.addParam will need to be updated to pass an object.

This includes methods in the following classes:

  • iDataQueue
  • iNetwork
  • iObj
  • iProd
  • iUserSpace
  • iWork

Copy link
Member

@kadler kadler left a comment

Choose a reason for hiding this comment

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

I think this is a bit overly complicated. Basically, addData should do something like this:

if type is ds:
  create "<ds>" node and set options accordingly
  foreach field in ds fields:
     xml += addData(field)
  append "</ds>"
else:
  create "<data>" node and set options accordingly

Comment on lines 28 to 35
['', '10i0', 0],
['', '10i0', 0],
['', '36h', ''],
['', '10A', ''],
['', '1A', ''],
['', '1A', ''],
['', '10i0', 0],
['', '10i0', 0],
Copy link
Member

Choose a reason for hiding this comment

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

Don't these all need to be objects now?

Copy link
Member Author

@abmusse abmusse Mar 24, 2020

Choose a reason for hiding this comment

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

Just to confirm we are on the same page.

How do we want DS to be defined?

Do we want DS to be an array of objects like shown here?
#107 (comment)

const errno = [
  // fields can be defined using objects as well
  // attributes are set on the object itself, not in a separate object
  {name: 'bytes_provided', type:'10i0', setlen: 'rec2' },
  
  {name: 'bytes_available', type: '10i0'},

  // The value can be set on the object
  {name: 'msgid', type: '7a', value: 'XXX0000'},

  // we can also omit names in object format 
  {type: '1h'},

  // Data structures can also be nested using object format
  {name: 'message fields', type='ds', fields=[
      // You can always mix and match object/array fields
      ['field1', '10i0'],
      {name: 'field2', type:'10i0'}
  ]}
];

If that is the case then yes we will need to change these to objects.

Copy link
Member

Choose a reason for hiding this comment

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

yes, all data types should be objects. We can add shortcuts in v1.1 or later.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, that example was before we talked about the problem for ds options. Thus, ds that need options must be objects, but I suggest we just stop supporting arrays for now as I just mentioned.

So something like this, instead:

const errno = {
    name: "errno",
    type: "ds",
    setlen: "whatever",
    fields: [
        # fields here
    ]
}

Copy link
Member Author

@abmusse abmusse Mar 24, 2020

Choose a reason for hiding this comment

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

but I suggest we just stop supporting arrays for now as I just mentioned.

Thanks! I agree all data types will just be objects for now. We can add shortcuts and options later.

I will also need to update the iPgm wrapper to create ds types as objects.

Copy link
Member

Choose a reason for hiding this comment

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

#157 will be affected as well.

Comment on lines 129 to 130
const nameNode = parameter.name ? ` name='${parameter.name}'` : '';
this.xml += `<parm${nameNode}`;
Copy link
Member

Choose a reason for hiding this comment

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

Please do this like parameter.io and parameter.by are done below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks will do

*/
__addData__(parameter = {}) {
// adding a data structure with data nodes
if (Array.isArray(parameter.value)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we still handling arrays here? Let __addDsNodes__ handle this. Also, I thought we were switching to object-only anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, never mind, I see that we are just checking if it's a ds or not. That should be done by checking parameter.type == 'ds' instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok will update to check for parameter.type == 'ds'

* @param {string} value
* @param {object} options
*/
__addDataNode__(name, type, value, options) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why this function is needed. It should be rolled in to __addData__.

* @param {array} ds
* @param {object} options
*/
__addDsNodes__(ds, options = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why this function is needed. It should be rolled in to __addData__.

@abmusse abmusse requested a review from kadler March 25, 2020 00:32
@@ -22,21 +22,23 @@
const { expect } = require('chai');
const { ProgramCall } = require('../../lib/itoolkit');

// now in this format
// [name, type, value, options]
Copy link
Member

Choose a reason for hiding this comment

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

This is outdated with the latest changes

@@ -71,7 +73,7 @@ describe('ProgramCall Class Unit Tests', () => {
error: 'fast',
});

pgm.addParam(outBuf, { io: 'out' });
pgm.addParam({ fields: outBuf, io: 'out', type: 'ds' });
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer the fields go last; seems most sensible to me.

];

pgm.addParam(params, { name: 'inds', by: 'val', io: 'both' });
pgm.addParam({
fields: params, type: 'ds', name: 'inds', by: 'val', io: 'both',
Copy link
Member

Choose a reason for hiding this comment

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

Prefer to see attribute order as name, then type, then the rest. fields probably makes most sense to go last

@abmusse abmusse requested a review from kadler March 25, 2020 18:19
@kadler kadler merged commit 376cc5a into IBM:master Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: Add Internal Function to handle adding Data Structure Parameters
2 participants