Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add
run_command_async
#993base: main
Are you sure you want to change the base?
Add
run_command_async
#993Changes from 18 commits
520a958
a913189
497979e
c5c8d17
2d2801b
adb445f
60bf783
a58121a
bb1bc1e
9ed29b7
6014b51
cb73715
c1dc017
5787db5
d28a9e2
3709f6d
d101914
5c433eb
ec3004d
447d6e7
7bcd6d7
8b16ece
9d3a052
78a1ad6
38c7cc5
0c8566b
aaff4f5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 57 in crates/bitwarden-c/src/c.rs
Codecov / codecov/patch
crates/bitwarden-c/src/c.rs#L39-L57
Check warning on line 72 in crates/bitwarden-c/src/c.rs
Codecov / codecov/patch
crates/bitwarden-c/src/c.rs#L61-L72
Check warning on line 74 in crates/bitwarden-c/src/c.rs
Codecov / codecov/patch
crates/bitwarden-c/src/c.rs#L74
Check warning on line 76 in crates/bitwarden-c/src/c.rs
Codecov / codecov/patch
crates/bitwarden-c/src/c.rs#L76
Check warning on line 112 in crates/bitwarden-c/src/c.rs
Codecov / codecov/patch
crates/bitwarden-c/src/c.rs#L108-L112
Check warning on line 117 in crates/bitwarden-c/src/c.rs
Codecov / codecov/patch
crates/bitwarden-c/src/c.rs#L115-L117
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I barely know what I did here, I need the time feature but only in debug, is this the right way to go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but I can't say I'm a fan of different behaviors on debug and release builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like the exact thing a debug build is for, it helps you write tests to make sure you've built the language wrapper correctly. That said I was able to move it into the
bitwarden-json
crate instead ofcore
.Check warning on line 39 in crates/bitwarden-core/src/platform/client_platform.rs
Codecov / codecov/patch
crates/bitwarden-core/src/platform/client_platform.rs#L29-L39
Check warning on line 48 in crates/bitwarden-core/src/platform/client_platform.rs
Codecov / codecov/patch
crates/bitwarden-core/src/platform/client_platform.rs#L42-L48
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these needed in the core crate? Consider moving them to a separate test crate, or bitwarden-c, or remove it completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved them to
bitwarden-json
, if I moved it intobitwarden-c
I would have to double parse the input string to check for special commands. Do you think that is alright?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just expose two regular functions for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cancellation test is mostly testing the very code written in
run_command_async
so we'd have to expose a method with very similar internals to that and repeat ourselves. The error test is testing that the C# wrapper properly handles how errors are given back to it through the bitwarden-json crate.Check warning on line 110 in crates/bitwarden-json/src/client.rs
Codecov / codecov/patch
crates/bitwarden-json/src/client.rs#L102-L110
Check warning on line 195 in crates/bitwarden-json/src/command.rs
Codecov / codecov/patch
crates/bitwarden-json/src/command.rs#L195
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative way. of doing this would be to add one or two separate commands to bitwarden-c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @bitwarden/team-secrets-manager-dev decide if they want to accept net 8 as the new target.