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

refactor(rust): refactored identity show command to use rpc abstraction #3668

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

divyank-aggarwal
Copy link
Contributor

Current Behavior

The current implementation in identity show command is calling std::process::exit, which is something we want to stop using to handle errors properly and give the user a better-formatted output when a command fails.

Proposed Changes

Relating to issue #3562

  1. Changed the implementation to use the node_rpc method as being used in subscription command
  2. Added implementations of ockam_command::util::output for ShortIdentityResponse and LongIdentityResponse to maintain current code quality and style
  3. Marked 2 functions that were being used before as allow(unused) to prevent linting errors.

Checks

Copy link
Member

@adrianbenavides adrianbenavides left a comment

Choose a reason for hiding this comment

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

Looking great @divyank-rs! A couple of things to address:

  1. You can delete the unused methods
  2. Can you add the bats test you mentioned here?

Copy link
Member

@adrianbenavides adrianbenavides left a comment

Choose a reason for hiding this comment

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

Awesome, thank you so much @divyank-rs for your contribution! 🎉

@mrinalwadhwa
Copy link
Member

@divyank-rs thank you for an awesome first contribution 🥳

@mergify mergify bot merged commit 5485a38 into build-trust:develop Oct 17, 2022
@mrinalwadhwa mrinalwadhwa added HACKTOBERFEST-ACCEPTED-2022 and removed HACKTOBERFEST-ACCEPTED To be used when a PR is ready to merge or after it's merged labels Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor identity show command to use rpc abstraction and to stop mapping errors manually
3 participants