Skip to content
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

typeStorageを確実に参照するようにする #1230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

forcia-tinoue
Copy link

alteaに対するxmlToObjectで配列型を配列として変換してくれない問題を暫定的に解消。
node-soap的に望ましい実装ではなさそうだが...。

@w666
Copy link
Collaborator

w666 commented May 1, 2024

To make it easier to understand could you translate this to English and explain why do you need this change? Also, if it is possible please cover changes with the tests.

@smokhov
Copy link
Contributor

smokhov commented Oct 4, 2024

From deepl:

"""
Ensure that typeStorage is referenced

Tentatively solved the problem that xmlToObject for ltea does not convert array types as arrays.
This may not be a desirable implementation from a node-soap point of view.
"""

Not sure if that helps 🤷 but I was curious :)

const typeName: string = type.name;
if(typeName in typeStorage){// typeにtypeNameが入ってしまった場合の対応、本来はこのようなことが起きないように対処すべきかもしれない。
Copy link
Contributor

Choose a reason for hiding this comment

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

"// typeName in type, perhaps this should be handled to prevent this from happening in the first place."

const typeName: string = type.name;
if(typeName in typeStorage){// typeにtypeNameが入ってしまった場合の対応、本来はこのようなことが起きないように対処すべきかもしれない。
if (this.$ref) {
element = typeStorage[typeName];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably, need to check that typeStorage[typeName] has a value, could be undefined, null ... anything

Copy link
Collaborator

@w666 w666 left a comment

Choose a reason for hiding this comment

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

Needs a test. While code makes sense, implementation should be locked, so other changes do not break this,

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.

3 participants