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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage crate reorg and cleanup #194

Merged
merged 4 commits into from
Mar 24, 2021
Merged

Storage crate reorg and cleanup #194

merged 4 commits into from
Mar 24, 2021

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?

sdk/storage/Cargo.toml Outdated Show resolved Hide resolved
sdk/storage/Cargo.toml Outdated Show resolved Hide resolved
@@ -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

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)

sdk/storage/src/blob_storage/blob/mod.rs Outdated Show resolved Hide resolved
@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!

@@ -98,8 +98,8 @@ fn format<D: Display>(value: D) -> String {
pub fn to_str_without_bom(bytes: &bytes::Bytes) -> Result<&str, std::str::Utf8Error> {
let s = from_utf8(bytes)?;

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