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

Add support for user-installable applications #327

Merged
merged 32 commits into from
Apr 6, 2024

Conversation

VelvetToroyashi
Copy link
Contributor

No description provided.

@VelvetToroyashi
Copy link
Contributor Author

VelvetToroyashi commented Mar 19, 2024

I've attempted to fix the failing unit tests, however it's complaining that the fields aren't surviving being serialized and then deserialized, to which I'm not entirely sure how to fix.

Issue was I didn't update the remaining samples. Fixing that has shown me that I have other things I need to add and fix.

@Nihlus
Copy link
Member

Nihlus commented Apr 2, 2024

Please rebase this onto the latest commit so you get all the required ReSharper inspection checks.

This may be non-functional at time of commit!
N.B: It is important to note that Remora allows:
- Automatic framework responses
- Those responses to be made ephemeral

Because of this, it brings into question whether there should be
validation on the account that a command may be marked as ephemeral,
(or worse, may be marked as NON-ephemeral) when the callback hint would
indicate that the opposite is true.

This behavior- or lack thereof- is technically spawned from developer
error, despite being the framework's decision on how to handle commands.
BREAKING-CHANGE: This changes the constructor of the `AllowedContextsAttribute` to take a `params ApplicationCommandContextType[]` instead of an `IReadOnlyList`
This commit is broken, as the Remora.Rest dependency needs to be updated
to the latest version to fix one (1) test.
@VelvetToroyashi
Copy link
Contributor Author

Well, messed up my commit dates, but at least it's rebased now. :v

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

InspectCode found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@MazeXP
Copy link
Contributor

MazeXP commented Apr 2, 2024

You might want to add the [PublicAPI] attribute on the interfaces ;)

Copy link
Member

@Nihlus Nihlus left a comment

Choose a reason for hiding this comment

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

Looks good - just a few comments, and you should probably run the data scrubber to ensure conformity with the rest of the test data.

@Nihlus Nihlus merged commit 178574c into Remora:main Apr 6, 2024
4 checks passed
@Nihlus Nihlus mentioned this pull request May 2, 2024
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