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

feat: watch register behalf #41

Merged
merged 9 commits into from
Aug 18, 2023
Merged

feat: watch register behalf #41

merged 9 commits into from
Aug 18, 2023

Conversation

chris13524
Copy link
Member

Description

  • Add watch_register_behalf() function which allows passing-through register_auth payload from client
  • Non-successful RPC requests will include the error body
  • just fmt will auto-format instead of just --check
  • get_message_id() refactor left-over from feat: expose message_id #39

Resolves #40

How Has This Been Tested?

With archive refactor

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@chris13524 chris13524 self-assigned this Aug 15, 2023
Copy link
Collaborator

@heilhead heilhead left a comment

Choose a reason for hiding this comment

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

Looks good! Added some comments.

@@ -280,7 +288,11 @@ impl Client {
let status = result.status();

if !status.is_success() {
return Err(HttpClientError::InvalidHttpCode(status).into());
let body = match result.text().await {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd use result.text().await.unwrap_or_else(|err| format!("... error calling result.text(): {err:?}")). Also the message is a bit strange. Maybe it should be something like 'error reading response body'?

Copy link
Member Author

@chris13524 chris13524 Aug 16, 2023

Choose a reason for hiding this comment

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

I don't know how I forgot about that function, fixed in 9729b6a

Three dots was suppose to clearly separate a body from a body error, but probably unnecessary.

Actually, I just pushed another commit that uses the Result value directly: fec5ef6. Much better IMO

@@ -66,7 +66,7 @@ fmt:

if command -v cargo-fmt >/dev/null; then
echo '==> Running rustfmt'
cargo +nightly fmt --all -- --check
cargo +nightly fmt --all
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the change? This was used for validation, along with clippy to make everything is all right before pushing. Formatting is usually done on the fly by the editor.

Copy link
Member Author

Choose a reason for hiding this comment

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

My editor (vscode) doesn't format all of the way that cargo fmt does, so cargo fmt sometimes does extra stuff or I miss it in a certain file.

I use 1 command in my devloop, something like just fmt clippy test. So when I get a formatting error then I have to manually run cargo fmt. I'd rather it fix my formatting for me, rather than tell me it's wrong.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. It hasn't been a problem for me, ever, in VSCode. Maybe you forgot to enable the +nightly feature in your VSCode addons?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can check this, but either way, wouldn't it be better to auto-format instead of just checking? What if my VS Code is out-of-sync with my CI/just tooling?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, as long as we still check formatting on the CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely! No doubt about that :)

@chris13524 chris13524 merged commit ad4a145 into main Aug 18, 2023
7 checks passed
@chris13524 chris13524 deleted the feat/watch_register_behalf branch August 18, 2023 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: watch register behalf
2 participants