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: NodeId change for class DataValue #1786

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

torarvid
Copy link
Contributor

@torarvid torarvid commented Feb 6, 2025

According to ObjectIds, DataValue = 23, and not 25 as it was before this commit. 25 is DiagnosticInfo, which can also be seen on line 1137 in the uatypes.py file.

(this is my first PR in this repo, and I found it while "just browsing around", so feel free to tell me if I'm completely off here 😉)

Edit: Now using ObjectIds.DataValue instead of 23 to make it more obvious.

According to ObjectIds, DataValue = 23, and not 25 as it was before this
commit. 25 is DiagnosticInfo, which can be seen below...
@oroulet
Copy link
Member

oroulet commented Feb 6, 2025

how can such a big bug been around for so long. I am not really getting it. is that attribute not used?

@torarvid
Copy link
Contributor Author

torarvid commented Feb 6, 2025

Btw, I noticed this "just by luck", since I thought it was surprising that the value 25 was used in both DataValue and DiagnosticInfo (they are close together in the file).

But it also got me wondering: why are plain numbers used here? Would it not be easier to spot/prevent bugs like these if the ObjectIds were used? So:

    data_type = NodeId(Int32(ObjectIds.DiagnosticInfo))

instead of

    data_type = NodeId(25)

@oroulet
Copy link
Member

oroulet commented Feb 6, 2025

historical reason I suppose. Also you probably do not need to cast to Int32. it is done at lower level I suppose.
Crazy that it works with wrong data type for DataValue.... but OK DataValue are probably mostly sent at places where the data type is hardcoded

@torarvid
Copy link
Contributor Author

torarvid commented Feb 6, 2025

Also you probably do not need to cast to Int32

Yeah, I just did it since it made my neovim happier:

image

@oroulet
Copy link
Member

oroulet commented Feb 6, 2025

ok then add it. The warning msut come from mypy or the facebook alternative

@torarvid
Copy link
Contributor Author

torarvid commented Feb 6, 2025

@oroulet Oh, did you mean I should change the PR so that instead of using 23 directly, I should just use the ObjectId? Sure, I can do that 👍🏼

(And I think actually it's my Language Server — pyright — that's giving the Int32 error, not mypy)

@oroulet oroulet merged commit 772316d into FreeOpcUa:master Feb 6, 2025
6 checks passed
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.

2 participants