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
[PM-10941] Add ssh import #82
[PM-10941] Add ssh import #82
Changes from 3 commits
0f8a7ac
822d0c0
c8e1801
53bf274
8488c40
b2b9740
799bc00
389d776
b21255a
2af2a63
3eab559
3341c5b
1172d03
90f5cea
edbba77
2653a8a
03ed7eb
31a401c
8858a2d
f68b747
643de0f
015cd8e
5afaae6
184a009
8b0352e
929797a
ca98c48
8d70e02
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Check warning on line 13 in crates/bitwarden-ssh/src/error.rs
Codecov / codecov/patch
crates/bitwarden-ssh/src/error.rs#L13
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.
โ @Hinton - Is there a path forward for i18n in the SDK? These seem like the kind of errors we'd like to forward to the user, which would mean communicating the i18n key we'd like the UI to use for localization along with the error.
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.
No we leave that to the presentation layer for now. Using i18n in lower level services have always been sort of a crutch.
For WASM we support https://sdk-api-docs.bitwarden.com/bitwarden_error_macro/attr.bitwarden_error.html which exposes basic, flat and full. For non user facing errors it's safe to use
basic
orflat
and log it. For user facing errors we generally encourage you to useflat
orfull
, and properly handle the error in the presentation layer.In this case if we want to show error messages to the user we should use
flat
orfull
and capture the different variants in the JS world and apply localization there.Note: The story on mobile is slightly different due to some uniffi limitations. We're working on providing the same capabilities there.
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.
Thanks! I'm going to leave this item open for now, but it's not blocking the review.
Check warning on line 9 in crates/bitwarden-ssh/src/generator.rs
Codecov / codecov/patch
crates/bitwarden-ssh/src/generator.rs#L8-L9
Check warning on line 19 in crates/bitwarden-ssh/src/generator.rs
Codecov / codecov/patch
crates/bitwarden-ssh/src/generator.rs#L16-L19
Check warning on line 31 in crates/bitwarden-ssh/src/generator.rs
Codecov / codecov/patch
crates/bitwarden-ssh/src/generator.rs#L31