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

fix: ReferenceNodes usage with mask set #683

Merged
merged 5 commits into from
Aug 29, 2023

Conversation

sruehl
Copy link
Contributor

@sruehl sruehl commented Aug 24, 2023

resolves #682
resolves #550

@sruehl
Copy link
Contributor Author

sruehl commented Aug 25, 2023

Another way would be to add a factory function to node.go accepting a ExpandedNode. Might be a bit more efficient.

@kung-foo
Copy link
Member

I think your idea for a static factory function sounds cleanest. It would make it clear what is happening, and who should be using it.

@sruehl sruehl force-pushed the fix/reference_nodes_mask_fix branch from f20bc2f to 5f8da7d Compare August 29, 2023 07:39
@sruehl
Copy link
Contributor Author

sruehl commented Aug 29, 2023

ok, I rebased the branch on main (not sure if that was necessary) and added a commit migrating this to the factory function (method). Instead adding it as a static I added it to client analogous to the c.Node call. This could also be moved to node.go and made be private if the exposed NodeFromExpandedNodeID doesn't make sense outside the ReferencedNodes call

@kung-foo kung-foo changed the title fix: fix ReferenceNodes usage with mask set fix: ReferenceNodes usage with mask set Aug 29, 2023
@kung-foo kung-foo added the bug Something isn't working label Aug 29, 2023
@kung-foo
Copy link
Member

Can you add a few tests to cover the conversion?

@sruehl
Copy link
Contributor Author

sruehl commented Aug 29, 2023

sure will do

@sruehl
Copy link
Contributor Author

sruehl commented Aug 29, 2023

731d545 contains the most relevant test where the arguments ensure the flag is set and the conversion ensure those are unset

Copy link
Member

@kung-foo kung-foo left a comment

Choose a reason for hiding this comment

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

Tested with Prosys Sim 5.4.6 and Azure IoTEdge 1.4.371.60

@kung-foo kung-foo merged commit 47bf562 into gopcua:main Aug 29, 2023
5 checks passed
@sruehl sruehl deleted the fix/reference_nodes_mask_fix branch August 29, 2023 11:55
@magiconair magiconair modified the milestones: v0.5.3, v0.5.2 Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with using ReferencedNodes Running examples/browse.go returns EOF error
3 participants