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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/user apps #336

Merged
merged 26 commits into from
May 17, 2024
Merged

Fix/user apps #336

merged 26 commits into from
May 17, 2024

Conversation

VelvetToroyashi
Copy link
Contributor

No description provided.

@VelvetToroyashi VelvetToroyashi marked this pull request as draft April 29, 2024 19:28
@VelvetToroyashi VelvetToroyashi marked this pull request as ready for review April 29, 2024 20:27
@Nihlus
Copy link
Member

Nihlus commented May 2, 2024

You're still missing test payloads for the added entities and tests for those.

@VelvetToroyashi
Copy link
Contributor Author

For interaction objects? Or the application object

@Nihlus
Copy link
Member

Nihlus commented May 2, 2024

The JSON payloads under test haven't been updated for existing entities (IApplicationCommand) and no new JSON payloads have been added for new types (like IMessageInteractionMetadata). Check your changed and added entities from both this PR and #327.

.WithElement(0, e => e.Is("1"))
.WithElement(1, e => e.Is("0"))
a => a.WithProperty("0", p => p.IsObject())
.WithProperty("1", p => p.IsObject())

Check warning

Code scanning / InspectCode

Variable in local function hides variable from outer scope Warning

Parameter 'p' hides outer parameter with the same name
This removes the string enum converter for ApplicationIntegrationType,
which caused deserialization to fail due to the fact that Discord
accepted a stringified value, but would return a numeric value in the
response payload, which the enum converter cannot handle.

Missing JSON fields (`contexts`, `integration_types` have also been
added, as they were previously missing, and likely would've caught
this issue.

A new converter type was also added to fix this confounding issue, but
it breaks on `Optional<IReadOnlyDictionary<...>>` which confuses me.

This is being posted purely for debugging purposes, and is known
to be broken at this point for reasons entirely beyond me and perhaps
even God.
[AllowedContexts(InteractionContextType.PrivateChannel)]
[DiscordInstallContext(ApplicationIntegrationType.UserInstallable)]
[Description("Posts a cat image that matches the user, usable exclusively in DMs.")]
public Task<IResult> PostUserHttpCatDMAsync([Description("The user to cattify")] IPartialUser catUser)

Check notice

Code scanning / InspectCode

Type member is never used: Non-private accessibility Note

Method 'PostUserHttpCatDMAsync' is never used
Copy link
Contributor

@MazeXP MazeXP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some small things. :)

BREAKING-CHANGE: Removes the name field from interaction metadata;
Discord says this field is currently unstable.
@Nihlus
Copy link
Member

Nihlus commented May 17, 2024

You've got a few alerts remaining along with some merge conflicts that came in as part of polls - other than that, things seem ready to go.

@VelvetToroyashi
Copy link
Contributor Author

@Nihlus should be good to go now

@Nihlus Nihlus merged commit 7ef0cc5 into Remora:main May 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants