-
Notifications
You must be signed in to change notification settings - Fork 20
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
Helpers fix #126
Draft
nayr7
wants to merge
15
commits into
krisppurg:master
Choose a base branch
from
nayr7:helpers-fix-1
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Helpers fix #126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ame templates due to overloading bug (`Webhook.edit()` -> `Webhook.editWebhook()`)
…e templates due to overloading bug (`Guild.edit()` -> `Guild.editGuild()`; `Guild.edit(GuildScheduledEvent)` -> `Guild.editGSE()`
…mand.editAC()` due to template overloading bug
…Client` is now a no-op.
…edit()` -> `Interaction.editInteraction()`; `GuildChannel.edit()` -> `GuildChannel.editChannel()`
Oops, didnt update the examples. I'll patch them |
91 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
implementation fixes
Some helpers were preventing code to compile because of bad implementation causing compiler errors. Those were fixed in this PR.
template renaming
Some helpers have been renamed because of overloading too much which was especially the case of the
edit
identifier which confused the compiler sometimes.getClient
andmainClient
In some Nim versions, users may encounter "client not found" or "client not registered" errors due to compiler bugs that have now been resolved. As a result, this feature works on recent versions of the compiler but not on older ones.
mainClient
is now a no-op. We can reserve it for future versions of the library in case we need it again (reverting to the old behavior or implementing another solution mentioned down below). We can also deprecate it if we decide that it's no longer necessary.getClient
now works by finding the 's' symbol which is always available within event handlers or command handlers like indimscmd
. It also checks if the 's' symbol is a Shard in case the user (for some obscure reason) decides to shadow it. This should work fine in older versions since it relies on less complex features of the language.However, this means we can no longer use the helpers outside of event or command handlers, for example after this patch it won't be possible to use helpers with a client that has
rest_mode = true
because they cannot use event handlers. I decided the fix was worth it because it makes the most common use-cases more reliable.One idea I had to mitigate this problem is to modify
mainClient
again to generate a template that aliases the user's client, allowing us to use the real symbol by callinggetClient
without relying on the old method again:Example usage:
Future work
Because nim templates are lazy we have to use them so that they are instantiated, the compiler cannot tell us if the template is good otherwise. So a CI/test-suite will follow-up this PR in order to test if they work correctly.