Skip to content

Conversation

MindFlavor
Copy link
Contributor

@MindFlavor MindFlavor commented Mar 22, 2021

This is a seemly big PR but it's mostly code shuffling. With this PR I believe we are very close to release v0.1! 🥇

Purpose

  1. Renamed the modules in blob_storage, table_storage, etc... to avoid module inception (see https://rust-lang.github.io/rust-clippy/master/index.html#module_inception).
  2. Moved the blob_storage specific clients from core to the appropriate module.
  3. Detangled the modules so the features compile independently (fixes Can't build azure_storage without default features or combinations of features less than the default #177). Right now by default it compiles everything but you can, say, use table_storage without anything else.
  4. Removed hardcoded dependency from hyper (to support target wasm & wasi too #37). 🍾
  5. Removed leftover dependencies no longer used.
  6. Clippy'd everything. Most changes are really simple (ok_or_else etc...). The only "significant" change is the switch from Into to From. Now the crate is clippy-compliant. 🍾
  7. Removed a lot of old code (most of it unnecessary, see below).

Cons

  1. To remove the hyper dependency I had to remove the ability to generate signed URIs. We will reenable it in a future PR.

@MindFlavor MindFlavor added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) project_structure labels Mar 22, 2021
Copy link
Contributor

@rylev rylev 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. I'm just unsure about the renaming of the submodules to have the _storage suffix. I'd like to hear the reasoning for that. I see you're trying to avoid module inception, but can you point to an example of that in the current code?

@@ -1,5 +1,5 @@
use azure_core::prelude::*;
use azure_storage::blob::prelude::*;
use azure_storage::blob_storage::prelude::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reasoning behind adding the storage suffix to all the submodules?

Copy link
Contributor Author

@MindFlavor MindFlavor Mar 24, 2021

Choose a reason for hiding this comment

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

I've explained below, it's for the module inception: tight now there is a blob module inside the blob module. Now we have the blob module inside the blob_storage module (which I think makes sense)

@MindFlavor
Copy link
Contributor Author

For example, the current master has a module table inside a module called table. Clippy complains with:

image

I cannot think of a better solution than appending storage but I'm open to suggestions.

@rylev
Copy link
Contributor

rylev commented Mar 24, 2021

@MindFlavor putting the the table model into a module called model would also fix this issue. I'm not against what you've done here, just trying to think about which solution is better.

Copy link
Contributor

@rylev rylev left a comment

Choose a reason for hiding this comment

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

😍 awesome!


Ok(if s.starts_with('\u{FEFF}') {
&s[3..]
Ok(if let Some(stripped) = s.strip_prefix('\u{FEFF}') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think the following is clearer:

s.strip_prefix('...').unwrap_or(s)

@rylev rylev merged commit 3e9955f into master Mar 24, 2021
@rylev rylev deleted the reorg/storage/pr branch March 24, 2021 20:12
@heaths heaths added the design-discussion An area of design currently under discussion and open to team and community feedback. label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. Storage Storage Service (Queues, Blobs, Files)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Can't build azure_storage without default features or combinations of features less than the default

3 participants