add support for edit_current_application endpoint #3035

ivinjabraham · 2024-11-13T20:21:03Z

jamesbt365

Use the provided to_vec function instead of importing it from serde_json.

jamesbt365 · 2024-11-13T20:28:14Z

Also you haven't added anything but the low level http interface? Http usually shouldn't be directly used and you should probably add a builder like how our other stuff interacts with http.

ivinjabraham · 2024-11-13T20:43:44Z

Use the provided to_vec function instead of importing it from serde_json.

Pesky auto-import. Should be fixed now.

Also you haven't added anything but the low level http interface? Http usually shouldn't be directly used and you should probably add a builder like how our other stuff interacts with http.

Right, so I'll make a edit_current_application_info.rs and impl Builder along with the required structs and methods?

ivinjabraham · 2024-11-15T09:16:47Z

It seems the CurrentApplicationInfo struct is out of date. There are few fields such as event_webhook_url and event_webhook_status that are missing. I'll send in a PR for that first.

Doc for reference

jamesbt365

edit_current_appllication_info.rs -> edit_current_application_info.rs

mkrasnitski · 2024-11-20T17:25:17Z

src/builder/edit_current_application_info.rs

+    /// Interactions endpoint URL for the app.
+    ///
+    /// To update an Interactions endpoint URL via the API, the URL must be valid according
+    /// to the [Receiving an Interaction]
+    /// (https://discord.com/developers/docs/interactions/receiving-and-responding#receiving-an-interaction) documentation.


These docs are private and therefore unnecessary. I'd recommend instead incorporating them into the docs for the methods themselves.

Ah, of course. Fixed it now.

GnomedDev · 2024-11-20T17:13:23Z

src/model/application/mod.rs

-    pub terms_of_service_url: Option<FixedString>,
+    pub bot: User,
    #[serde(default)]
-    pub privacy_policy_url: Option<FixedString>,
-    pub owner: Option<User>,
-    // omitted `summary` because it deprecated
-    pub verify_key: FixedString,
+    pub terms_of_service_url: Option<String>,
+    #[serde(default)]
+    pub privacy_policy_url: Option<String>,
+    #[serde(default)]
+    pub owner: User,
+    pub verify_key: String,
    pub team: Option<Team>,
    #[serde(default)]
    pub guild_id: Option<GuildId>,
    #[serde(default)]
+    pub guild: Option<PartialGuild>,
+    #[serde(default)]
    pub primary_sku_id: Option<SkuId>,
    #[serde(default)]
-    pub slug: Option<FixedString>,
+    pub slug: String,
    #[serde(default)]
-    pub cover_image: Option<FixedString>,
+    pub cover_image: String,
    #[serde(default)]
    pub flags: Option<ApplicationFlags>,
    #[serde(default)]
-    pub tags: Option<Vec<String>>,
+    pub approximate_guild_count: Option<u32>,
    #[serde(default)]
-    pub install_params: Option<InstallParams>,
+    pub approximate_user_install_count: Option<u32>,
+    #[serde(default)]
+    pub redirect_uris: Vec<String>,
    #[serde(default)]
-    pub custom_install_url: Option<FixedString>,
+    pub interactions_endpoint_url: Option<String>,
    /// The application's role connection verification entry point, which when configured will
    /// render the app as a verification method in the guild role verification configuration.
-    pub role_connections_verification_url: Option<FixedString>,
+    #[serde(default)]
+    pub role_connections_verification_url: Option<String>,
+    #[serde(default)]
+    pub event_webhooks_url: Option<String>,
+    pub event_webhook_status: EventWebhookStatus,
+    #[serde(default)]
+    pub event_webhook_type: Vec<EventWebhookType>,
+    #[serde(default)]
+    pub tags: Option<Vec<String>>,
+    #[serde(default)]
+    pub install_params: Option<InstallParams>,
    #[serde(default)]
    pub integration_types_config: HashMap<InstallationContext, InstallationContextConfig>,
-    pub approximate_guild_count: Option<u32>,
-    pub approximate_user_install_count: Option<u32>,
-    pub guild: Option<PartialGuild>,
-    pub redirect_uris: Option<Vec<String>>,
-    pub interactions_endpoint_url: Option<String>,
+    #[serde(default)]
+    pub custom_install_url: String,


You have replaced a bunch of FixedString here with String, I'm assuming it's just a leftover from when this PR targeted current, can you change it back to use FixedString (for performance reasons).

Yep, they were leftovers. I've changed them back to FixedString now.

GnomedDev · 2024-11-20T17:45:17Z

src/model/application/mod.rs

+impl InstallParams {
+    #[must_use]
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    #[must_use]
+    pub fn scopes(mut self, scopes: Vec<Scope>) -> Self {
+        self.scopes = FixedArray::from_vec_trunc(scopes);
+        self
+    }
+
+    #[must_use]
+    pub fn permissions(mut self, permissions: Permissions) -> Self {
+        self.permissions = permissions;
+        self
+    }
+}


Please make this an CreateInstallParams in the new module to avoid mixing memory-optimised (FixedArray) and compute-optimised (Vec) types. This would also allow for new to take both the required fields, scopes and permissions.

Made the requested changes, I've also squashed the two commits since they're intertwined now.

GnomedDev · 2024-11-20T17:46:11Z

src/builder/edit_current_application_info.rs

+    pub fn icon(mut self, hash: &str) -> Self {
+        if let Ok(image_hash) = hash.parse() {
+            self.icon = Some(image_hash);
+        } else {
+            println!("Error parsing image hash while constructing EditCurrentApplicationInfo");
+        }
+        self
+    }
+
+    /// Default rich presence invite cover image for the app.
+    pub fn cover_image(mut self, hash: &str) -> Self {
+        if let Ok(image_hash) = hash.parse() {
+            self.cover_image = Some(image_hash);
+        } else {
+            println!("Error parsing image hash while constructing EditCurrentApplicationInfo");
+        }
+        self
+    }


Please don't take types just to parse them into different types.

Please don't print on the user's behalf.

This is meant to be base64 image data, not an image hash, as the docs say.

I've removed the unnecessary code and changed it to accept base64 data instead.

jamesbt365 · 2024-11-27T16:04:00Z

I think everything in CurrentApplicationInfo can use FixedString (String replacment) and FixedArray (Vec replacement)

github-actions bot added the http Related to the `http` module. label Nov 13, 2024

ivinjabraham force-pushed the current branch from f298296 to 424af70 Compare November 13, 2024 20:21

jamesbt365 requested changes Nov 13, 2024

View reviewed changes

ivinjabraham force-pushed the current branch from 424af70 to 0cee4cc Compare November 13, 2024 20:29

ivinjabraham marked this pull request as draft November 13, 2024 20:34

ivinjabraham mentioned this pull request Nov 15, 2024

update current application object struct #3041

Closed

ivinjabraham changed the base branch from current to next November 15, 2024 15:09

ivinjabraham force-pushed the current branch 2 times, most recently from 5ba44d9 to 24ffd92 Compare November 15, 2024 15:45

GnomedDev force-pushed the next branch from 9296017 to efb5820 Compare November 15, 2024 19:29

ivinjabraham force-pushed the current branch from 24ffd92 to f2c1085 Compare November 17, 2024 12:24

github-actions bot added the model Related to the `model` module. label Nov 17, 2024

ivinjabraham force-pushed the current branch from 7f72d28 to 71ab298 Compare November 17, 2024 13:57

github-actions bot added the builder Related to the `builder` module. label Nov 17, 2024

ivinjabraham force-pushed the current branch 3 times, most recently from 20d45ab to 8fd6b7e Compare November 17, 2024 16:41

jamesbt365 requested changes Nov 17, 2024

View reviewed changes

ivinjabraham force-pushed the current branch 2 times, most recently from 8c6dd76 to 6a5d2a5 Compare November 17, 2024 18:04

ivinjabraham marked this pull request as ready for review November 18, 2024 03:43

ivinjabraham requested a review from jamesbt365 November 18, 2024 03:43

mkrasnitski reviewed Nov 20, 2024

View reviewed changes

ivinjabraham force-pushed the current branch from 6a5d2a5 to 8ed3e32 Compare November 20, 2024 17:44

GnomedDev requested changes Nov 20, 2024

View reviewed changes

ivinjabraham force-pushed the current branch from 8ed3e32 to 52057ba Compare November 27, 2024 15:59

ivinjabraham force-pushed the current branch 5 times, most recently from e871454 to d05677b Compare November 28, 2024 04:41

arqunis force-pushed the next branch from d7c5ee3 to 7e90b9d Compare December 8, 2024 18:51

ivinjabraham force-pushed the current branch from d05677b to a088f6e Compare December 9, 2024 14:37

ivinjabraham added 3 commits December 13, 2024 10:45

add support for edit_current_application endpoint

3e26b47

update current application object struct

a74aa1c

add builder for edit_current_application_info endpoint

1b63edf

ivinjabraham force-pushed the current branch from a088f6e to 1b63edf Compare December 13, 2024 05:16

ivinjabraham requested review from GnomedDev and mkrasnitski December 13, 2024 05:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for edit_current_application endpoint #3035

add support for edit_current_application endpoint #3035

ivinjabraham commented Nov 13, 2024 •

edited

Loading

jamesbt365 left a comment

jamesbt365 commented Nov 13, 2024

ivinjabraham commented Nov 13, 2024

ivinjabraham commented Nov 15, 2024 •

edited

Loading

jamesbt365 left a comment

mkrasnitski Nov 20, 2024

ivinjabraham Nov 20, 2024

GnomedDev Nov 20, 2024

ivinjabraham Nov 27, 2024

GnomedDev Nov 20, 2024

ivinjabraham Nov 28, 2024

GnomedDev Nov 20, 2024

ivinjabraham Nov 28, 2024

jamesbt365 commented Nov 27, 2024

add support for edit_current_application endpoint #3035

Are you sure you want to change the base?

add support for edit_current_application endpoint #3035

Conversation

ivinjabraham commented Nov 13, 2024 • edited Loading

jamesbt365 left a comment

Choose a reason for hiding this comment

jamesbt365 commented Nov 13, 2024

ivinjabraham commented Nov 13, 2024

ivinjabraham commented Nov 15, 2024 • edited Loading

jamesbt365 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesbt365 commented Nov 27, 2024

ivinjabraham commented Nov 13, 2024 •

edited

Loading

ivinjabraham commented Nov 15, 2024 •

edited

Loading