-
Notifications
You must be signed in to change notification settings - Fork 103
Convert input handling based on CCSID scenarios #2556
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
base: master
Are you sure you want to change the base?
Conversation
…arios. Signed-off-by: worksofliam <mrliamallan@live.co.uk>
👋 A new build is available for this PR based on 79e49f1. |
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Signed-off-by: worksofliam <mrliamallan@live.co.uk>
Tools.qualifyPath(inAmerican(file.library), inAmerican(file.name), inAmerican(member), asp, true), | ||
Tools.qualifyPath(inAmerican(file.library), inAmerican(file.name), inAmerican(member), undefined, true) | ||
Tools.qualifyPath(forSystem(file.library), forSystem(file.name), forSystem(member), asp, localVariants), | ||
Tools.qualifyPath(fromSystem(file.library), fromSystem(file.name), fromSystem(member), undefined, localVariants) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to be inAmerican
, so I think this should be forSystem
here.
Tools.qualifyPath(fromSystem(file.library), fromSystem(file.name), fromSystem(member), undefined, localVariants) | |
Tools.qualifyPath(fromSystem(file.library), forSystem(file.name), forSystem(member), undefined, localVariants) |
sysNameInAmerican(string: string, enabled: boolean = true) { | ||
if (enabled) { | ||
const fromChars = this.variantChars.local; | ||
const toChars = this.variantChars.american; | ||
|
||
let result = string; | ||
let result = string; | ||
|
||
for (let i = 0; i < fromChars.length; i++) { | ||
result = result.replace(new RegExp(`[${fromChars[i]}]`, `g`), toChars[i]); | ||
}; | ||
for (let i = 0; i < fromChars.length; i++) { | ||
result = result.replace(new RegExp(`[${fromChars[i]}]`, `g`), toChars[i]); | ||
}; | ||
|
||
return result; | ||
} | ||
|
||
return result | ||
return string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic behind the enabled
parameter is unclear or would need to be redefined. Looking at all the references in the code, it's always this class' qsysPosixPathsRequireTranslation
field that is used.
Here is a suggestion: make that second parameter optional, and when it's true
, consider qsysPosixPathsRequireTranslation
value. Then in the places where the method is called with the second parameter, pass ifNeeded
as true
:
sysNameInAmerican(string: string, enabled: boolean = true) { | |
if (enabled) { | |
const fromChars = this.variantChars.local; | |
const toChars = this.variantChars.american; | |
let result = string; | |
let result = string; | |
for (let i = 0; i < fromChars.length; i++) { | |
result = result.replace(new RegExp(`[${fromChars[i]}]`, `g`), toChars[i]); | |
}; | |
for (let i = 0; i < fromChars.length; i++) { | |
result = result.replace(new RegExp(`[${fromChars[i]}]`, `g`), toChars[i]); | |
}; | |
return result; | |
} | |
return result | |
return string; | |
} | |
sysNameInAmerican(string: string, ifNeeded?: boolean) { | |
if (!ifNeeded || this.qsysPosixPathsRequireTranslation) { | |
const fromChars = this.variantChars.local; | |
const toChars = this.variantChars.american; | |
let result = string; | |
for (let i = 0; i < fromChars.length; i++) { | |
result = result.replace(new RegExp(`[${fromChars[i]}]`, `g`), toChars[i]); | |
}; | |
return result; | |
} | |
return string; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@worksofliam I made an initial code review. I'll see how it goes at runtime.
I left a few comments for some changes. Thanks!
Enhance input conversion logic to account for CCSID in various contexts, ensuring proper translation for system commands and SQL statements. Adjustments made to improve path handling and user feedback during checks.
Fixes #2519
How to test