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

Bugfix/640 #641

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Bugfix/640 #641

wants to merge 4 commits into from

Conversation

kpalatzky
Copy link

Fixes #640

kpalatzky added 3 commits November 8, 2023 08:52
Changed createXMLReference to refToNodeType in nextSibling / prevSibling
Added XMLNode|null for prevSibling and nextSibling. All types excepct XMLDocument inherit from XMLNode and XMLDocument can not be returned in this case.
@kpalatzky
Copy link
Author

@rchipka The pipeline fails due to a GYP error. I cannot fix this error myself.

@rchipka
Copy link
Member

rchipka commented Nov 8, 2023

We shouldn't use refToNodeType because then we have to use a type assertion.

Instead we should check if the .next/.prev ref is null and return null, otherwise return createXMLReference(XMLNode,...), that way the return type will be inferred correctly.

@rchipka
Copy link
Member

rchipka commented Nov 8, 2023

Also if you could add a test for this that would be greatly appreciated

@rchipka
Copy link
Member

rchipka commented Nov 8, 2023

Hmm, although that does raise the question of how to handle casting to XMLElement in nextElement/prevElement

@kpalatzky
Copy link
Author

In principle, nextSibling / prevSibling should also return the correct element and not only nextElement / prevElement.

In principle, I think that refToNodeType is already correct. However, the document node should probably still be explicitly checked here so that the types are correct.

An XML element should always be returned for an element reference.

@kpalatzky
Copy link
Author

It also seems that the build on ubuntu does work fine. Whats reason why it does fail on macos? I am not familiar with GYP errors.

@rchipka
Copy link
Member

rchipka commented Nov 8, 2023

Seems like maybe a python version issue considering the error is related to unknown flags for an open() call

@rchipka
Copy link
Member

rchipka commented Nov 8, 2023

I agree that we should always return an instance that best matches the underlying node ref type, however I'm wondering if there's a way we can do this without a manual type assertion

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.

nextSibling/prevSibling always returns XMLNode
2 participants