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

Move options object arguments before function arguments in Command/EventListener #96

Open
eritbh opened this issue Aug 26, 2021 · 5 comments · May be fixed by #98
Open

Move options object arguments before function arguments in Command/EventListener #96

eritbh opened this issue Aug 26, 2021 · 5 comments · May be fixed by #98
Labels
changes: api modifies behavior of the public API quality removes something unnecessary or updates code style version: minor semver-minor, involves feature addition
Milestone

Comments

@eritbh
Copy link
Owner

eritbh commented Aug 26, 2021

Instead of

export default new Command('ping', msg => {
  // this could be long...
}, {
  guildOnly: true,
});

we should support putting options at the beginning before the potentially long function body:

export default new Command('ping', {
  guildOnly: true,
}, msg => {
  // this could be long...
});

Same goes for EventListener - especially important for this one since the once option changes the meaning of the listener body significantly.

Will require deprecation for the old form - both should be supported until v3. Runtime checks will need to be present past v3 to support the options being optional but coming before another required parameter.

old idea

shower thought: instead of

new EventListener('ready', /* ... */, {once: true})

it would be trivial to allow

new EventListener('ready:once', /* ... */);

Is this a good idea? I don't know of any other options we'd want to put on an event listener, so is the options object even necessary? I feel like allowing it via options is a cleaner way to do it, but requiring a full options object for a single property that will always be true or not present seems kinda eh. But I also don't think I would want the :ready method to be the only way to do it, because it's pretty different from how anything else handles one-time listeners.

As far as typings go it should be pretty easy I think - can make a new interface for the events that uses template literal string types for the additional keys I think

@eritbh eritbh added enhancement something doesn't happen that should changes: api modifies behavior of the public API version: minor semver-minor, involves feature addition labels Aug 26, 2021
@NotReallyEight
Copy link

Hmm, wouldn't it be less clean as an idea? I can't really think of a better way to do so, but the current system seems "good" in terms of how clean it can be

@eritbh
Copy link
Owner Author

eritbh commented Aug 30, 2021

I think my main concern with this is that if you're registering a complex listener, its easy to miss the once: true at the very bottom of the definition when reading the code back later. Commands have the same issue. I think a better way to handle both cases is to allow the options object to come before the function. Performing extra checks at runtime to allow for a different order of the arguments shouldn't be a big deal for performance since commands and event listeners are only registered once for the lifetime of the bot process.

I guess the other question is, do we want to enforce ordering the other way for v3, or does it make sense to allow both forms forever? I am slightly concerned that this would lead to confusion if the order of things isn't consistent.

@NotReallyEight
Copy link

I think you can like allow both at the release and deprecate the older way (or somehow idk), and in another release remove it completely and stay with the new order could be better

@eritbh
Copy link
Owner Author

eritbh commented Aug 30, 2021

Typescript doesn't like putting optional arguments before required ones, but it's easy enough to do it via overrides and some runtime shuffling of arguments. I definitely want the options argument to be optional - I think the readability benefits will generally be worth the extra overhead. My only concern is how this will look in the generated docs, because I don't want to confuse people with weird looking signatures in docs if possible. Will need to look into that.

@eritbh eritbh changed the title More concise way to specify once on event listeners Move options object arguments before function arguments in Command/EventListener Aug 30, 2021
@eritbh
Copy link
Owner Author

eritbh commented Aug 30, 2021

Committing to this. I got it working locally, and I can mark the messy implementation signature internal so it doesn't get included in docs. This is good enough for me.

@eritbh eritbh added this to the v2.4 milestone Aug 30, 2021
@eritbh eritbh linked a pull request Aug 30, 2021 that will close this issue
4 tasks
@eritbh eritbh added quality removes something unnecessary or updates code style and removed enhancement something doesn't happen that should labels Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: api modifies behavior of the public API quality removes something unnecessary or updates code style version: minor semver-minor, involves feature addition
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants