Checking of C-CONV

src/instance.rs

@@ -112,8 +112,9 @@ impl Instance {
 
     /// Creates a new [`Instance`] by trying to get the [relevant instance urls](UrlBundle) from a root url.
     /// Shorthand for `Instance::new(UrlBundle::from_root_domain(root_domain).await?)`.
-    ///
+    // RAGC: Can we really call this a conversion?
-    /// If `limited` is `true`, then Chorus will track and enforce rate limits for this instance.
+    // Would with_root_url be better? Not really I think, because with is for more details
+    // Where are this is with.. less? (or rather with other ones)
     pub async fn from_root_url(root_url: &str) -> ChorusResult<Instance> {
         let urls = UrlBundle::from_root_url(root_url).await?;
         Instance::new(urls).await

src/types/entities/attachment.rs

@@ -57,6 +57,7 @@ pub struct PartialDiscordFileAttachment {
 }
 
 impl PartialDiscordFileAttachment {
+    // RAGC: Is move_x proper naming?
     /// Moves `self.content` out of `self` and returns it.
     pub fn move_content(self) -> (Vec<u8>, PartialDiscordFileAttachment) {
         let content = self.content;

src/types/entities/config.rs

@@ -11,13 +11,21 @@ pub struct ConfigEntity {
 }
 
 impl ConfigEntity {
-    pub fn as_string(&self) -> Option<String> {
+    // RAGC: not sure about this, but it performs an "expensive" to_string opeartion, resulting in
+    // "borrowed -> owned" ownership
+    pub fn to_string(&self) -> Option<String> {
         let Some(v) = self.value.as_ref() else {
             return None;
         };
         Some(v.as_str().expect("value is not a string").to_string())
     }
 
+    // RAGC: Is this proper naming?
+    // If you check https://rust-lang.github.io/api-guidelines/naming.html#c-conv
+    //
+    // as_* should be "borrowed -> borrowed" ownership;
+    // This has "borrowed -> owned" ownership, yet isn't a to_*, because it isn't expensive.
+    // It seems the inner serde type has the same issue, so I am happy to just leave this be
     pub fn as_bool(&self) -> Option<bool> {
         let Some(v) = self.value.as_ref() else {
             return None;

src/types/entities/user.rs

@@ -26,7 +26,7 @@ pub struct UserData {
 }
 
 impl User {
-    pub fn to_public_user(self) -> PublicUser {
+    pub fn into_public_user(self) -> PublicUser {
         PublicUser::from(self)
     }
 }

src/types/utils/rights.rs

@@ -124,6 +124,7 @@ bitflags! {
 }
 
 impl Rights {
+    // FIXME: Why are any and has the same??
     pub fn any(&self, permission: Rights, check_operator: bool) -> bool {
         (check_operator && self.contains(Rights::OPERATOR)) || self.contains(permission)
     }
@@ -138,6 +139,9 @@ impl Rights {
     /// # Notes
     /// Unlike has, this returns an Error if we are missing rights
     /// and Ok(true) otherwise
+    // RAGC: Is this proper naming?
+    // I don't think it is mentioned anywhere how this should be named
+    // Also, isn't it redundant to return a bool, if it's always going to be true?
     pub fn has_throw(&self, permission: Rights) -> Result<bool, &'static str> {
         if self.has(permission, true) {
             Ok(true)