Simplify enum objects #39

modulatingforce · 2024-03-22T09:52:18-04:00

modulatingforce commented

2024-03-22 09:52:18 -04:00

Below example problem code

#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub enum ModType {
    BotModule(String),
}

We can probably have that simplified to

#[derive(Debug, PartialEq, Eq, Hash, Clone)]
pub struct BotModule(String)

I believe there was some reason I chose the enum path? But we'll document any issues here

This change will :

improve readability of code
- for a programmer, abstractions like ModType have no meaning if it's only variant is BotModules
not improve or optimize code
- therefore low priority

Plan of Action :

Refactor simple enum objects like BotModule to tuple structs (similar to above)
if required, core code might need to be adjusted for this new implementation

Below example problem code ```rust #[derive(Debug, PartialEq, Eq, Hash, Clone)] pub enum ModType { BotModule(String), } ``` We can probably have that simplified to ```rust #[derive(Debug, PartialEq, Eq, Hash, Clone)] pub struct BotModule(String) ``` --- I believe there was some reason I chose the enum path? But we'll document any issues here --- **This change will :** - improve readability of code - for a programmer, abstractions like `ModType` have no meaning if it's only variant is `BotModules` - not improve or optimize code - therefore low priority --- # Plan of Action : - [x] Refactor simple enum objects like `BotModule` to tuple structs (similar to above) - [x] if required, core code might need to be adjusted for this new implementation

modulatingforce commented

2024-03-22 09:55:18 -04:00

Related documentation

Using Tuple Structs Without Named Fields to Create Different Types

https://doc.rust-lang.org/book/ch05-01-defining-structs.html#using-tuple-structs-without-named-fields-to-create-different-types

Related documentation Using Tuple Structs Without Named Fields to Create Different Types https://doc.rust-lang.org/book/ch05-01-defining-structs.html#using-tuple-structs-without-named-fields-to-create-different-types

modulatingforce added the

Priority

Low

label 2024-03-22 10:41:08 -04:00

HaruYuumei commented

2024-03-22 16:06:44 -04:00

I tried my best 👍 I've runned the bot a couple times after the updates and it seens to be working, but Idk what other tests might be needed to fully see if everything works fine, also, there is a part in the code that can be further edited, in identity.rs line 590
pub struct IdentityManager { special_roles_users: Arc<RwLock<UserRolesDB>>, }
can be changed to
pub struct IdentityManager(Arc<RwLock<UserRolesDB>>);
but I don't know how to change the rest of the code to adjust to this change 😢
Also, I hope I didn't break everything Lule

I tried my best 👍 I've runned the bot a couple times after the updates and it seens to be working, but Idk what other tests might be needed to fully see if everything works fine, also, there is a part in the code that can be further edited, in identity.rs line 590 ` pub struct IdentityManager { special_roles_users: Arc<RwLock<UserRolesDB>>, } ` can be changed to ` pub struct IdentityManager(Arc<RwLock<UserRolesDB>>); ` but I don't know how to change the rest of the code to adjust to this change 😢 Also, I hope I didn't break everything Lule

HaruYuumei was assigned by modulatingforce

2024-03-22 20:24:34 -04:00

modulatingforce reviewed 2024-03-22 20:31:43 -04:00

modulatingforce left a comment

Hi @HaruYuumei

Great work! I still need to checkout the code for any other areas, but this is exactly what I was looking for

Could you look though on these two comments? Can it be set this way or were there issues setting it this way?

Thanks!

Hi @HaruYuumei Great work! I still need to checkout the code for any other areas, but this is exactly what I was looking for Could you look though on these two comments? Can it be set this way or were there issues setting it this way? Thanks!

src/core/botinstance.rs Outdated

					
				@ -34,0 +34,4 @@

				// pub use ChType::Channel;

				//

				//simplifying from enum to struct

				pub struct Channel(pub String);

modulatingforce commented

2024-03-22 20:30:06 -04:00

Does this work instead? I don't know if pub is required next to String

pub struct Channel(String)

Does this work instead? I don't know if `pub` is required next to `String` ```rust pub struct Channel(String) ```

modulatingforce commented

2024-03-23 13:56:44 -04:00

Preferring Haru's original (below) as this would allow a more intuitive use of this simple structure

pub struct Channel(pub String);

Preferring Haru's original (below) as this would allow a more intuitive use of this simple structure ```rust pub struct Channel(pub String); ```

modulatingforce marked this conversation as resolved

src/core/botmodules.rs Outdated

					
				@ -47,0 +45,4 @@

				//     BotModule(String),

				// }

				pub struct BotModule(pub String);

modulatingforce commented

2024-03-22 20:30:27 -04:00

Does this work instead? I don't know if pub is required next to String

pub struct BotModule(String)

Does this work instead? I don't know if `pub` is required next to `String` ```rust pub struct BotModule(String) ```

modulatingforce commented

2024-03-23 13:56:51 -04:00

Preferring Haru's original (below) as this would allow a more intuitive use of this simple structure

pub struct Channel(pub String);

Preferring Haru's original (below) as this would allow a more intuitive use of this simple structure ```rust pub struct Channel(pub String); ```

modulatingforce marked this conversation as resolved

modulatingforce commented

2024-03-22 20:35:25 -04:00

I tried my best 👍 I've runned the bot a couple times after the updates and it seens to be working, but Idk what other tests might be needed to fully see if everything works fine, also, there is a part in the code that can be further edited, in identity.rs line 590
pub struct IdentityManager { special_roles_users: Arc<RwLock<UserRolesDB>>, }
can be changed to
pub struct IdentityManager(Arc<RwLock<UserRolesDB>>);
but I don't know how to change the rest of the code to adjust to this change 😢
Also, I hope I didn't break everything Lule

Oh to add, uhh Yeah I think some of the other more complex structs could have the same ideas applies, but I'm going to hold off for now just in case. IdentityManager for example, I'm not sure just yet if I'm just sticking with that one attribute or not

So for the complex ones, we'll hold off for now

> I tried my best 👍 I've runned the bot a couple times after the updates and it seens to be working, but Idk what other tests might be needed to fully see if everything works fine, also, there is a part in the code that can be further edited, in identity.rs line 590 > ` > pub struct IdentityManager { > special_roles_users: Arc<RwLock<UserRolesDB>>, > } > ` > can be changed to > ` > pub struct IdentityManager(Arc<RwLock<UserRolesDB>>); > ` > but I don't know how to change the rest of the code to adjust to this change 😢 > Also, I hope I didn't break everything Lule Oh to add, uhh Yeah I think some of the other more complex structs could have the same ideas applies, but I'm going to hold off for now just in case. `IdentityManager` for example, I'm not sure just yet if I'm just sticking with that one attribute or not So for the complex ones, we'll hold off for now

modulatingforce commented

2024-03-22 20:48:34 -04:00

👍yeah no other areas of improvement. I thought there was more enum and struct than I originally thought

If the comments from #39 (comment) can be addressed first, we can likely go ahead with the merge

👍yeah no other areas of improvement. I thought there was more `enum` and `struct` than I originally thought If the comments from https://git.flake.sh/modulatingforce/forcebot_rs/pulls/39#issuecomment-629 can be addressed first, we can likely go ahead with the merge

modulatingforce added this to the Rust Learning project 2024-03-22 21:39:38 -04:00

modulatingforce added the

Status

Need More Info

label 2024-03-23 14:02:07 -04:00

modulatingforce commented

2024-03-23 14:03:36 -04:00

Looking good @HaruYuumei ! I've set this to Need More Info in case you have any questions or want to review before we merge and delete the branch

If ready to merge, remove the WIP flag, and we'll merge it in

Looking good @HaruYuumei ! I've set this to `Need More Info` in case you have any questions or want to review before we merge and delete the branch If ready to merge, remove the WIP flag, and we'll merge it in

HaruYuumei commented

2024-03-23 14:25:16 -04:00

Looks like its working fine for now :) I think it can be merged 👍

HaruYuumei changed title from ~~WIP: Simplify enum objects~~ to Review: Simplify enum objects

2024-03-23 14:26:09 -04:00

modulatingforce force-pushed Simplify-objects from 333bd34866 to 503910b252

2024-03-23 14:52:59 -04:00

Compare

modulatingforce changed title from ~~Review: Simplify enum objects~~ to Simplify enum objects

2024-03-23 14:55:16 -04:00

modulatingforce removed the

Status