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

feat: precondition guides #91

Merged
merged 34 commits into from
Dec 10, 2021
Merged

feat: precondition guides #91

merged 34 commits into from
Dec 10, 2021

Conversation

Lioness100
Copy link
Contributor

@Lioness100 Lioness100 commented Nov 14, 2021

I was deliriously tired whilst writing these so don't expect them to be perfectly thorough, structurally sound, or grammatically correct. But, I thought it would be a good idea to get the ball rolling. Note that I made Creating your first precondition the first guide because it includes an intro to what preconditions are. I also made a new page, Reporting precondition failures, the name of which is obviously eh at best. Brain vomit:

  • In the cooldown section, should milliseconds be represented as 10 * 1000, 10000, or 10_000?
  • Could I make a PR in /framework to change the cooldown precondition error from saying "You have just used this command..." to "This command has just been used"? This would be more accurate when the scope is set to anything other than USER, because they might've not just used the command. Furthermore, it might be worth changing the "just" part as well, since the cooldown could be 1 hour for all we know
  • Should the guides use the BucketScope enum, or just stick with strings?
  • Should the guides use the CommandOptionsRunTypeEnum enum, or just stick with strings?
  • The runIn JSDoc description shows lowercase and invalid values. PR goes brrr?
  • There are some highlighting and link errors in the listener guides. Should I group some fixes into this PR for convenience or create a new one?
  • Should listeners in the guide use generics? I recall you guys saying it would be removed, but shouldn't we put them in until it is?
  • Speaking of the listeners' guide, maybe "What are listeners" should be merged into "Creating your first listener"? Or I separate some precondition explaining into a "What are preconditions" for consistency
  • Lowercase starting comments in "Getting Started" = inconsistency = oh it hurts my brain :( Should I group some fixes into this PR for convenience or create a new one?
  • The name of the section in commands is "Creating a basic command", but in the others, it's "Creating your first listeners/preconditions/arguments" = inconsistency = oh it hurts my brain :(

Closes #93

Edit by favna: striken through text is resolved

@vercel
Copy link

vercel bot commented Nov 14, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sapphiredev/website/AuQ1PT3wt4jUW1MpWrPPQ2ES36pr
✅ Preview: https://website-git-fork-lioness100-feat-preconditionguides-sapphiredev.vercel.app

@Lioness100
Copy link
Contributor Author

Vercel isn't letting me see why the deployment failed :(

@UndiedGamer
Copy link
Contributor

Can you do yarn build and send the ss of errors?

@Lioness100
Copy link
Contributor Author

There's a bunch of errors about broken links, although all of the ones mentioned do actually work. I'm not sure if that would cause a deployment to fail, though.
image

I also got this error, but I assumed that that was because I didn't have all the git modules downloaded locally and that it would be fixed when pushed to this repo somehow idk
image

docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

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

Could I make a PR in /framework to change the cooldown precondition error from saying "You have just used this command..." to "This command has just been used"?

Sure!

Furthermore, it might be worth changing the "just" part as well, since the cooldown could be 1 hour for all we know

How about:

There is a cooldown in effect for this command. It can be used again in <time>

Should the guides use the BucketScope enum, or just stick with strings?

The enum

Should the guides use the CommandOptionsRunTypeEnum enum, or just stick with strings?

The enum

The runIn JSDoc description shows lowercase and invalid values. PR goes brrr?

Probably still from v1. Yes please.

There are some highlighting and link errors in the listener guides. Should I group some fixes into this PR for convenience or create a new one?

New PR. This one is already becoming fairly large.

Should listeners in the guide use generics? I recall you guys saying it would be removed, but shouldn't we put them in until it is?

@kyranet showed @vladfrangu how to make them work. I still have no idea how. If this is true however then I suppose we're keeping them. And in that case it would be good to add them.

Speaking of the listeners' guide, maybe "What are listeners" should be merged into "Creating your first listener"? Or I separate some precondition explaining into a "What are preconditions" for consistency

I prefer separate "what are" pages at the top of the collapsible group. It can be a short page with a quick overview of what they are for people who are new and so unfamiliar to the terms.

Lowercase starting comments in "Getting Started" = inconsistency = oh it hurts my brain :( Should I group some fixes into this PR for convenience or create a new one?

Group it with the other PR for listeners in a general fixes PR

The name of the section in commands is "Creating a basic command", but in the others, it's "Creating your first listeners/preconditions/arguments" = inconsistency = oh it hurts my brain :(

Ditto above. "Creating your own...." is better.

docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
docs/Guide/preconditions/command-cooldown.mdx Show resolved Hide resolved
@vladfrangu
Copy link
Member

@kyranet showed @vladfrangu how to make them work. I still have no idea how. If this is true however then I suppose we're keeping them. And in that case it would be good to add them.

Basically, when you provide the generic, you don't get autocompletes for the params in the run method but TypeScript will error when you define a param with a wrong type.

Try it! Make a messageCreate listener and try typing the message as a number

Copy link
Member

@favna favna left a comment

Choose a reason for hiding this comment

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

#97 has been merged and this PR has been rebased. Please migrate your <Tabs for JS/ESM/TS to the next plugin based syntax

@Lioness100
Copy link
Contributor Author

all my homies hate git

@Lioness100
Copy link
Contributor Author

what have i done

docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
docs/Guide/preconditions/command-cooldown.mdx Outdated Show resolved Hide resolved
Co-authored-by: Jeroen Claassens <[email protected]>
@favna favna merged commit 78b98d5 into sapphiredev:main Dec 10, 2021
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.

5 participants