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

zwave-js: module init, zwave-js-server: init at 1.33.0 #230380

Merged
merged 1 commit into from
Nov 6, 2023
Merged

zwave-js: module init, zwave-js-server: init at 1.33.0 #230380

merged 1 commit into from
Nov 6, 2023

Conversation

graham33
Copy link
Contributor

@graham33 graham33 commented May 6, 2023 •
edited
Loading

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@graham33 graham33 marked this pull request as draft May 6, 2023 19:02
@graham33 graham33 requested a review from mweinelt May 6, 2023 19:02
@graham33 graham33 changed the title zwave-js-server: module init zwave-js: module init May 7, 2023
@graham33 graham33 force-pushed the feature/zwave-js-server_module branch 3 times, most recently from eca4923 to a93649d Compare May 13, 2023 22:16
@graham33
Copy link
Contributor Author

Almost ready for review, I just need to work out a clean way to get a dummy serial port for testing.

@graham33 graham33 force-pushed the feature/zwave-js-server_module branch 2 times, most recently from 3dc4a46 to b99d299 Compare May 14, 2023 11:59
@graham33 graham33 changed the title zwave-js: module init zwave-js: module init, zwave-js-server: init at 1.28.0 May 14, 2023
@graham33 graham33 force-pushed the feature/zwave-js-server_module branch from b99d299 to 252ea8c Compare May 14, 2023 12:21
@graham33 graham33 marked this pull request as ready for review May 14, 2023 12:27
@graham33
Copy link
Contributor Author

Almost ready for review, I just need to work out a clean way to get a dummy serial port for testing.

Done (turns out zwave-js-server has a built-in --mock-driver option).

@graham33 graham33 force-pushed the feature/zwave-js-server_module branch from 252ea8c to ac8d18c Compare May 14, 2023 18:47
@graham33 graham33 force-pushed the feature/zwave-js-server_module branch from ac8d18c to cb4519c Compare June 3, 2023 16:03
@graham33
Copy link
Contributor Author

graham33 commented Jun 3, 2023

@mweinelt congrats on 23.05! Anything further needed on this one, do you think?

@graham33
Copy link
Contributor Author

I think the unfortunate interface design choice is that serialPort may be null, which will trigger the assertion instead of the standard error, that a required option is not set.

Ideally the mock driver wouldn't be a user-facing option in the first place, and could be configured in the test using extraFlags. That option is also something a regular user could benefit from.

I guess that could work, the tests, or anyone actually wanting to use the mock driver (probably nobody I guess!), would have to pass a dummy value for serialPort which wouldn't be used, I suppose. I'll try to implement it this way.

@graham33 graham33 changed the title zwave-js: module init, zwave-js-server: init at 1.31.0 zwave-js: module init, zwave-js-server: init at 1.33.0 Oct 30, 2023
@graham33 graham33 force-pushed the feature/zwave-js-server_module branch 2 times, most recently from 9bafdb1 to 2c12945 Compare October 30, 2023 20:47
@graham33
Copy link
Contributor Author

This should be ready for another review, thanks.

@graham33 graham33 force-pushed the feature/zwave-js-server_module branch from 2c12945 to 6f3ac1e Compare October 30, 2023 21:05
@graham33 graham33 force-pushed the feature/zwave-js-server_module branch from f67cc77 to 4cb1e83 Compare November 1, 2023 21:49
@graham33 graham33 requested a review from h7x4 November 1, 2023 21:49
@graham33
Copy link
Contributor Author

graham33 commented Nov 1, 2023

All your suggestions added @h7x4 , thanks!

@graham33 @mweinelt @h7x4
zwave-js: module init, zwave-js-server: init at 1.33.0
b2fccae 
Co-authored-by: Martin Weinelt <mweinelt@users.noreply.github.com>
Co-authored-by: h7x4 <h7x4@nani.wtf>
@graham33 graham33 force-pushed the feature/zwave-js-server_module branch from 4fb2ab5 to b2fccae Compare November 2, 2023 08:21
@graham33
Copy link
Contributor Author

graham33 commented Nov 2, 2023

@mweinelt addressed your suggestions, thanks.

@mweinelt
Copy link
Member

mweinelt commented Nov 3, 2023

@dotlambda Can you take a look or dismiss your review?

@dotlambda dotlambda dismissed their stale review November 4, 2023 09:14

No expertise in the software.

@graham33
Copy link
Contributor Author

graham33 commented Nov 4, 2023

Looks like this is mergeable, assuming @h7x4 doesn't have any further comments?

@graham33
Copy link
Contributor Author

graham33 commented Nov 6, 2023

@mweinelt it'd be great to merge this if we can, before it rots again!

@mweinelt mweinelt merged commit a3708ce into NixOS:master Nov 6, 2023
19 checks passed
@graham33
Copy link
Contributor Author

graham33 commented Nov 7, 2023

Wooh thanks @mweinelt and all the people who helped review! 🎉

@graham33 graham33 deleted the feature/zwave-js-server_module branch November 7, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mweinelt mweinelt mweinelt approved these changes

@dotlambda dotlambda Awaiting requested review from dotlambda

@h7x4 h7x4 Awaiting requested review from h7x4

Assignees
No one assigned
Labels
6.topic: nixos 6.topic: nodejs 8.has: changelog 8.has: documentation 8.has: module (new) 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@graham33 @mweinelt @dotlambda @h7x4