MrGVSV / bevy_proto

Create config files for entities in Bevy
Other
239 stars 25 forks source link

fix some clippy errors #11

Closed B-Reif closed 2 years ago

B-Reif commented 2 years ago

Running cargo clippy currently comes up with a few errors. I fixed some of them in this commit.

A few others come up that I wanted to mention:

I also don't know how strongly you are attached to your current rustfmt.toml settings, but I would prefer to follow rust community style-guides where possible.

MrGVSV commented 2 years ago

This is great! Thanks for this.

A few others come up that I wanted to mention:

  • Errors around borrowed boxes. I think we can replace some of the &Box fields with just & where clippy tells us to.
  • Errors around tabs in doc comments. This can cause some issues for generated doc pages.

Yeah I'm fine with fixing these. I meant to get around to the tabs issue for a while but forgot 😅

I also don't know how strongly you are attached to your current rustfmt.toml settings, but I would prefer to follow rust community style-guides where possible.

No attachment. Let's switch to the community style-guidelines 🙂

B-Reif commented 2 years ago

No attachment. Let's switch to the community style-guidelines 🙂

Cool, we can do this in a separate PR since that's a lot of lines to touch.

B-Reif commented 2 years ago

This type is giving a clippy error about type complexity:

/// A resource containing data for all prototypes that need data stored
pub struct ProtoData {
    /// Maps Prototype Name -> Component Type -> HandlePath -> Asset Type -> HandleUntyped
    handles: HashMap<
        String, // Prototype Name
        HashMap<
            TypeId, // Component Type
            HashMap<
                String, // Handle Path
                HashMap<
                    Uuid,          // Asset UUID
                    HandleUntyped, // Handle
                >,
            >,
        >,
    >,
    prototypes: HashMap<String, Box<dyn Prototypical>>,
}

Do we need to access each layer of this mapping individually? If not we could save some allocations by consolidating the keys into one struct that we could hash on its own. I was thinking something like:

#[derive(Hash)]
pub struct ProtoHandleKey {
  name: String,
  typeId: TypeId,
  path: String,
  uuid: Uuid
}

pub struct ProtoData {
  handles: HashMap<ProtoHandleKey, HandleUntyped>,
  // ...
}
MrGVSV commented 2 years ago

Do we need to access each layer of this mapping individually? If not we could save some allocations by consolidating the keys into one struct that we could hash on its own.

Yeah, we only care about the handle at the end there. so this would be a good change. I think it's relevant to this PR so you can just add it to this one rather than opening another.

(As you can see, this was one of my first Rust projects so it has a lot of little improvements that could be made like this haha)

B-Reif commented 2 years ago

Yeah, we only care about the handle at the end there. so this would be a good change. I think it's relevant to this PR so you can just add it to this one rather than opening another.

On second thought, the nested maps actually make more sense because we can key into them with just references. Making a new key type would mean calling .to_string() just to construct keys for lookups. To shut clippy up I just made a type alias for the innermost map.

MrGVSV commented 2 years ago

Yeah, we only care about the handle at the end there. so this would be a good change. I think it's relevant to this PR so you can just add it to this one rather than opening another.

On second thought, the nested maps actually make more sense because we can key into them with just references. Making a new key type would mean calling .to_string() just to construct keys for lookups. To shut clippy up I just made a type alias for the innermost map.

That's true. I'd still like to find a way to clean this up in the future. Maybe once I get the reflection rework done, we can see about tidying it up further.

What if we actually create additional types for the others (i.e., type PathUuidMap = HashMap<String, UuidHandleMap>)?

B-Reif commented 2 years ago

It's just aliases, I don't think it really makes it much clearer to do that. If you think it helps readability then we can.

For now all of the clippy errors are cleaned up 👍