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.

Sign up for GitHub

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

Jump to bottom

update opcua modules - remove unnecessary data transformation #1072

Closed
wants to merge 9 commits into from
Closed

update opcua modules - remove unnecessary data transformation #1072

wants to merge 9 commits into from

Conversation

erossignon
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Sep 6, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2e6fc09) 75.67% compared to head (ba3dcf4) 75.67%.
Report is 3 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1072   +/-   ##
=======================================
  Coverage   75.67%   75.67%           
=======================================
  Files          80       80           
  Lines       16159    16159           
  Branches     1519     1519           
=======================================
  Hits        12228    12228           
  Misses       3889     3889           
  Partials       42       42
Files Coverage Δ
...ackages/binding-opcua/src/opcua-protocol-client.ts 79.74% <50.00%> (ø)

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

danielpeintner
danielpeintner approved these changes Sep 6, 2023
View reviewed changes
Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

LGTM

relu91
relu91 requested changes Sep 6, 2023
View reviewed changes
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.

Can you try to avoid those changes in the package-lock? I'm afraid they will randomly appear in the future if we do not fix them now.

package-lock.json Outdated Show resolved Hide resolved
@erossignon erossignon force-pushed the UpdateOPCUA branch 4 times, most recently from 7e3bcea to be0c3c8 Compare September 7, 2023 17:24
relu91
relu91 reviewed Sep 7, 2023
View reviewed changes
package-lock.json Outdated Show resolved Hide resolved
relu91
relu91 reviewed Sep 8, 2023
View reviewed changes
package-lock.json Outdated Show resolved Hide resolved
relu91
relu91 reviewed Sep 8, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
@danielpeintner
Copy link
Member

danielpeintner commented Sep 12, 2023 •
edited by relu91
Loading

FYI: I created a new node-wot release with newer dependencies. Maybe you can simply align (by merging in master) the versions there... not sure if it helps

@danielpeintner
Copy link
Member

danielpeintner commented Oct 4, 2023 •
edited
Loading

To bring this PR forward I plan to split this PR in several PRs to fix/update each part separately.
I hope this is fine by everyone @erossignon

danielpeintner added a commit to danielpeintner/thingweb.node-wot that referenced this pull request Oct 13, 2023
@danielpeintner
refactor(binding-opcua): remove unnecessary data transformation
6346679 
see eclipse-thingweb#1072
@danielpeintner danielpeintner mentioned this pull request Oct 13, 2023
Merged
danielpeintner added a commit to danielpeintner/thingweb.node-wot that referenced this pull request Oct 13, 2023
@danielpeintner
chore(binding-opcua): improve/stabilize tests
95ed97c 
see eclipse-thingweb#1072
This was referenced Oct 13, 2023
Merged
Merged
@danielpeintner
Copy link
Member

danielpeintner commented Oct 13, 2023 •
edited
Loading

I opened 3 follow-up PRs:

Once they are merged we can close this one...

danielpeintner added a commit that referenced this pull request Oct 13, 2023
@danielpeintner
refactor(binding-opcua): remove unnecessary data transformation (#1119)
d6fdbdf 
see #1072
danielpeintner added a commit that referenced this pull request Oct 13, 2023
@danielpeintner @JKRhb
chore(binding-opcua): improve/stabilize tests (#1120)
2e6fc09 
* chore(binding-opcua): improve/stabilize tests

see #1072

* fix typo

Co-authored-by: Jan Romann <jan.romann@uni-bremen.de>

---------

Co-authored-by: Jan Romann <jan.romann@uni-bremen.de>
@erossignon
Copy link
Contributor Author

To bring this PR forward I plan to split this PR in several PRs to fix/update each part separately. I hope this is fine by everyone @erossignon

Is this really necessary ? At the end the code will be idential in the repo

@erossignon
Copy link
Contributor Author

erossignon commented Oct 13, 2023 •
edited
Loading

I see some conflits, I'll rebase.
and take the opportinite to split the commit into 2 or 3 contextualized commit.

@erossignon erossignon force-pushed the UpdateOPCUA branch 3 times, most recently from 3a39b66 to e246446 Compare October 13, 2023 17:02
@danielpeintner
Copy link
Member

danielpeintner commented Oct 16, 2023 •
edited
Loading

Is this really necessary ? At the end the code will be idential in the repo

We (and most other projects) have the policy to create one PR per issue/change. Putting a lot of stuff in 1 PR is not useful and makes it

  • more difficult to review
  • more difficult to approve a simple change as it can be blocked by other changes (as happened in this case)
  • more difficult to know what has been changed based on 1 PR
  • etc etc

I hope you can see our rational...

By now the other PRs have been merged and I will close this one. Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielpeintner danielpeintner danielpeintner approved these changes

@relu91 relu91 Awaiting requested review from relu91 relu91 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@erossignon @danielpeintner @relu91