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

refactor(utilities): Use relative timestamps #993

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

ravener
Copy link
Contributor

@ravener ravener commented Mar 13, 2024

I think a much more user-friendly option is to use relative timestamps, it makes more sense to say you can use the command in 5 seconds, or in 6 hours, instead of telling the user that you can use it at 6:42 PM.

On the other hand I'm not sure if this change is to be merged as is, it's meant to start a discussion:

  • Does the noun until still make any sense here after the change?
  • Maybe we could add another substitution like {time} or {duration} and keep {until} as is, this will preserve backward compatibility with existing code while giving an option to use this relative notation for those who desires it.
  • If we go with that, I'd still choose the relative notation to be the default option as it makes more sense.
  • I would also choose to refactor the default reply message as it kind of feels unprofessional to me.

Off topic but I would also propose for making the ephemeral option be true by default, it's one of the best features of slash commands and as slash commands are meant to reduce user mistakes it also makes sense to spit out rate limit errors and similar in an ephemeral reply rather than contributing to useless bot command spam, at least that's my opinion that all errors and embarrassing mistakes remain an ephemeral reply only sending the actual results to the world. Obviously I can just wrap the guard into a custom function and set it to true as the default for my own bot but I'd like to hear the possibility of it being the default here?

@ravener ravener requested a review from vijayymmeena as a code owner March 13, 2024 11:30
@vijayymmeena
Copy link
Member

@ravener can you send example of how it look before vs now. I just want to see, how it's look for end user. Thank you

@vijayymmeena
Copy link
Member

Also run npm run fix:prettier

@vijayymmeena
Copy link
Member

@ravener can you send example of how it look before vs now. I just want to see, how it's look for end user. Thank you

oh sorry, I just woke up and came from reading email title only. Just get the prettier done. Awesome change btw.

@vijayymmeena
Copy link
Member

@ravener yeah, making errors and similar ephemeral true is good idea, I say go with it in another PR.

@ravener
Copy link
Contributor Author

ravener commented Mar 13, 2024

Also run npm run fix:prettier

done, my bad I quickly used the github editor but moving forward i should stick to the tooling

vijayymmeena
vijayymmeena previously approved these changes Mar 13, 2024
@vijayymmeena
Copy link
Member

@ravener there is relevant examples of rate limit. They are affected by this. Could you please address them as well.

@vijayymmeena
Copy link
Member

image

Relative is good, but we cannot control it.

@ravener
Copy link
Contributor Author

ravener commented Mar 13, 2024

image

Relative is good, but we cannot control it.

yeah I figured it'd look a bit weird when the time expires (it starts showing 'ago') but thought that'd be a minor inconvenience and the benefits would still outweigh it. In my own bots I typically use relative dates but without discord's timestamps so they may not update in real time but they look better, especially for short cooldowns like 5 seconds.

@ravener there is relevant examples of rate limit. They are affected by this. Could you please address them as well.

The examples in the README look okay unless I'm missing something?

@vijayymmeena
Copy link
Member

@ravener I am going to save your time, I will take over this PR (hope you don't mind).

I am implementing a new useful utility called TimeFormat. I will use that in this PR. One of its function is TimeFormat.StaticRelativeTime, so we can make that as a default time format. Or let me know, what do you think.

@ravener
Copy link
Contributor Author

ravener commented Mar 13, 2024

@ravener I am going to save your time, I will take over this PR (hope you don't mind).

I am implementing a new useful utility called TimeFormat. I will use that in this PR. One of its function is TimeFormat.StaticRelativeTime, so we can make that as a default time format. Or let me know, what do you think.

Sounds good to me 👍, looking forward to it.

@vijayymmeena vijayymmeena merged commit 426361f into discordx-ts:main Mar 13, 2024
4 checks passed
This was referenced Mar 13, 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