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

chore(binding-opcua): enable strict type checking #1057

Merged

Conversation

danielpeintner
Copy link
Member

another one of #758

Note: since at the moment I am not able to test OPCUA locally due to network/company restriction I will use the CI for testing.

@danielpeintner danielpeintner changed the title refactor: enable strict type checking refactor: enable strict type checking for OPCUA Aug 11, 2023
@danielpeintner danielpeintner changed the title refactor: enable strict type checking for OPCUA chore(binding-opcua): enable strict type checking Aug 11, 2023
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage: 45.45% and project coverage change: -0.10% ⚠️

Comparison is base (3a74512) 75.38% compared to head (732baa6) 75.28%.
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1057      +/-   ##
==========================================
- Coverage   75.38%   75.28%   -0.10%     
==========================================
  Files          80       80              
  Lines       15443    15455      +12     
  Branches     1477     1481       +4     
==========================================
- Hits        11641    11636       -5     
- Misses       3766     3783      +17     
  Partials       36       36              
Files Changed Coverage Δ
...ackages/binding-opcua/src/opcua-protocol-client.ts 79.90% <23.07%> (-1.14%) ⬇️
packages/binding-opcua/src/codec.ts 86.94% <77.77%> (-0.09%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielpeintner
Copy link
Member Author

I think the PR is now ready for review...

@erossignon may I ask you to take a look?
Maybe you can also explain the reason for #1057 (review)

@danielpeintner
Copy link
Member Author

@erossignon seems to be busy. Anyhow, the changes seem simple enough to go on... what do others think?

@relu91
Copy link
Member

relu91 commented Sep 5, 2023

Thinking on the point, the only reason that I see to clone dataValue might be to sanitize the Object and remove functions and methods. Can we still keep this conversion and mark it as an issue? 🤔

Just to keep the current idempotent with the previous implementation.

to sanitize the object and remove functions
@danielpeintner
Copy link
Member Author

danielpeintner commented Sep 5, 2023

Just to keep the current idempotent with the previous implementation.

Yes, let's keep it as it was, see 732baa6
I don't think an issue is really needed.

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Good to go

@relu91 relu91 merged commit 949a7e1 into eclipse-thingweb:master Sep 5, 2023
8 checks passed
@erossignon
Copy link
Contributor

Sorry, I didn't find time to look into this; but you did well any, we can always look at why this JSON.parse(JSON.stringify(opcuaJsonEncodeDataValue(dataValue, true))); is needed.
I think we can simplify this with the most recent version node-opcua-json package that improved the opcuaJsonEncodeDataValue. but that's a different story.
I'll propose a PR.

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