Skip to content

Added types implementation - #77

Open
alexandrahrastnik wants to merge 5 commits into
IBM:masterfrom
alexandrahrastnik:master
Open

Added types implementation#77
alexandrahrastnik wants to merge 5 commits into
IBM:masterfrom
alexandrahrastnik:master

Conversation

@alexandrahrastnik

Copy link
Copy Markdown

Suggested fix for #75

Signed-off-by: HRASTNIK Patrick Ing patrick.hrastnik@holter.at

Suggested fix for IBM#75
Signed-off-by: HRASTNIK Patrick Ing <patrick.hrastnik@holter.at>

@kadlerkadler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please resolve the merge conflicts and re-submit.

Comment threadlib/itoolkit.js
Comment on lines +576 to +581
packed(digits, delimiter) {
return digits.toString() + "P" + delimiter.toString();
},
zoned(digits, delimiter) {
return digits.toString() + "S" + delimiter.toString();
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd suggest changing digits and delimiter to something more descriptive. SQL uses the terms precision and scale, though some may find that confusing. Perhaps total_digits and decimal_digits would be more appropriate?

Comment threadlib/itoolkit.js
Comment on lines +588 to +589
float(digits, delimiter) {
return digits.toString() + "F" + delimiter.toString();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would rather see separate float and double functions which take no parameters and return "4F2" and "8F4", respectively.

Aside: I'm not sure why XMLSERVICE wants 4F2 and 8F4, when RPG says the decimal digits field must be blank (and would be 4F and 8F, respectively).

Comment threadlib/itoolkit.js Outdated
Comment threadlib/itoolkit.js Outdated
Comment threadlib/itoolkit.js Outdated
Comment threadlib/itoolkit.js Outdated
Comment threadlib/itoolkit.js
Comment on lines +582 to +587
signed(digits, delimiter) {
return digits.toString() + "I" + delimiter.toString();
},
unsigned(digits, delimiter) {
return digits.toString() + "U" + delimiter.toString();
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AFAIK, XMLSERVICE doesn't support integer types with scale, so the delimiter must always be 0 and is thus unnecessary.

Also, I'd also like to have separate functions for each type/length that would need no parameters, eg. signed_byte, signed_short, signed_int, signed_bigint, etc..

alexandrahrastnikand others added 4 commits October 4, 2019 20:18
Applied suggestion from @kadler
Co-Authored-By: Kevin Adler <kadler@us.ibm.com>
Applied suggestion from @kadler
Co-Authored-By: Kevin Adler <kadler@us.ibm.com>
Applied suggestion from @kadler
Co-Authored-By: Kevin Adler <kadler@us.ibm.com>
Applied suggestion from @kadler
Co-Authored-By: Kevin Adler <kadler@us.ibm.com>
@github-actions

Copy link
Copy Markdown

👋 Hi! This pull request has been marked stale due to inactivity. If no further activity occurs, it will automatically be closed.

@kadler

Copy link
Copy Markdown
Member

Would be good to get this merged. @patrickhrastnik, not sure if you have time to resolve the conflicts otherwise @abmusse if you want to pull the changes in to a new PR and get it merged that would be great.

@kadler

kadler commented Jan 14, 2021

Copy link
Copy Markdown
Member

One thing that I was thinking about that would impact this, is it would be really nice to have a varchar type, but that would require having the function/object return both the XMLSERVICE type as well as the varying="XX" attribute.

I think this could be done by having the function return an object with the attributes and using the javascript spread operator:

# oldwayprogram.addParam({type: '10A',varying: '4',value: 'Gill'});functionvarchar(len){return{type: len.toString()+'a',varying: '2'}}functionlongvarchar(len){return{type: len.toString()+'a',varying: '4'}}program.addParam({ ...longvarchar(10),value: 'Gill'});

If we did that, it might also make sense to use camel case on the names:

  • Integer
  • Char
  • Varchar
  • LongVarchar
  • etc

@abmusse

abmusse commented Jan 15, 2021

Copy link
Copy Markdown
Member

One thing that I was thinking about that would impact this, is it would be really nice to have a varchar type, but that would require having the function/object return both the XMLSERVICE type as well as the varying="XX" attribute.

I think this could be done by having the function return an object with the attributes and using the javascript spread operator:

# oldwayprogram.addParam({type: '10A',varying: '4',value: 'Gill'});functionvarchar(len){return{type: len.toString()+'a',varying: '2'}}functionlongvarchar(len){return{type: len.toString()+'a',varying: '4'}}program.addParam({ ...longvarchar(10),value: 'Gill'});

If we did that, it might also make sense to use camel case on the names:

  • Integer
  • Char
  • Varchar
  • LongVarchar
  • etc

Nice! The spread operator works out cleanly in this situation. I just tested it out in sample script:

functionLongVarChar(len){return{type: len.toString()+'A',varying: '4'}}functionaddParam(param){console.log(param)}addParam({type: '10A',varying: '2',value: 'Gill'});addParam({ ...LongVarChar(10),value: 'Gill'})

Output

{ type: '10A', varying: '2', value: 'Gill' }
{ type: '10A', varying: '4', value: 'Gill' }

The use of camel case be to minimize confusion for the reader?
I'm pretty sure the linter will complain but we can work around that.

@github-actions

Copy link
Copy Markdown

👋 Hi! This pull request has been marked stale due to inactivity. If no further activity occurs, it will automatically be closed.

@abmusseabmusse added keep-open Exempts stale action from auto closing the issue/pr. and removed stale labels Feb 16, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

keep-openExempts stale action from auto closing the issue/pr.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@alexandrahrastnik@kadler@abmusse