Azure / Azure/azure-sdk-for-rust

[Service Bus] Resolve PR #2863 review debt

Open
#2,293 0 comments 0 reactions 1 assignee Assigned to @RickWinter View on GitHub
Client Service Bus
Dominant language
Rust
Stars
884
Forks
365
Avg merge
2d 19h
Merged PRs (30d)
109

Description

## Summary

PR #2863 landed the initial `azure_messaging_servicebus` crate with a large set of unresolved review comments. This issue tracks those comments. It holds 43 items, and 38 are still open.

## Motivation

The review debt is not evenly spread. Thirteen of the 38 open items are the same argument about where the types live, what the crate exports, how the client is constructed, and which fields are public. That makes the public API shape the gate on the architecture board review, so those items should close before the session receiver, the rule manager, and the processor add more public types.

The items below are grouped by the issue that owns them. Each group closes with its owning issue rather than on its own.

## Proposal

Resolve the items in each group as part of the owning issue, then close this issue when all groups are clear.

### Error type (#4938)

- [ ] Fix ServiceBusError, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255312629 , https://github.com/Azure/azure-sdk-for-rust/pull/2863/files#r2271554197 , https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271554197 , https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271553225 , https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271551809)
- [ ] Unwrap failures, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258311431)
- [ ] Move error, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271566034)
- [ ] Cleanup Errors, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255312629)

### Recoverable connection and authorizer (#4940)

- [ ] Explore not holding the connection and session, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258189472)
- [ ] Review the timeouts, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288835060)
- [ ] Authorizer cleanup, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288839200)

### Public API shape (#4941)

- [ ] ReceiveMode copy, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258234837)
- [ ] Don't expose as Hashmap, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258319466)
- [ ] Mutability, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258322826)
- [ ] &str vs &String, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255420541)
- [ ] Review where types live and are exported, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271562699)
- [ ] Move clients, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271603478)
- [ ] Enable options, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271606333)
- [ ] Review where types live, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255318367)
- [ ] Cleanup exports, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2286456811)
- [ ] Change the builder pattern, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2286459626)
- [ ] SubClient cleanups, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2286463951)
- [ ] SubQueue surface area, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288819989)
- [ ] Move the builder, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288827077)

### Message model (#4942)

- [ ] Single message struct, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2257841558)
- [ ] AMQP message initilialization, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2257858451)
- [ ] Hashmap consistency, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258184706)
- [ ] Only convert binary, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258315774)
- [ ] Serialization, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271557725)

### Settlement (#4943)

- [ ] Dead letter cleanup, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288817741)

### Core AMQP crate (#4935, #4936)

- [ ] azure_core_amqp consolidation, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255421514)

### Connection string credentials (#4956)

- [ ] Connection string remnant, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288850082)

### Tests (#4957)

- [ ] Tracing simplication in tests, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258355215)
- [ ] Async test cleanup, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288852970)
- [ ] New resource cleanup, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288851133)

### Documentation and samples (#4959)

- [ ] az usage in crate readme and scripts, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271543930)
- [ ] Cleanup README to clarify where commands run, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271552598)
- [ ] Example boiler plate, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2271609637)
- [ ] Remove or cleanup Contributing.md, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2286449162)
- [ ] Trim the fluff in comments, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2286473109)

### Crate hygiene and constants (#4962)

- [ ] Remove deadcode, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258036898)
- [ ] Add manifest constants, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2258230824)

### Already resolved

- [x] ClientOptions field removal, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2288821560)
- [x] Bicep file cleanup, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255250470)
- [x] Bicep file move, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255248979)
- [x] Bicep file cleanup, see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2255254329)
- [x] Files should use '/', see (https://github.com/Azure/azure-sdk-for-rust/pull/2863#discussion_r2286450862)

## Related

Sub-issue of #4934.

The four open sub-issues of this issue are covered by the groups above. #2920 and #2921 belong to the error type group. #2923 belongs to the message model group. #2925 belongs to the recoverable connection group.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.