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(Commands): resolve mentions into objects #198

Merged
merged 11 commits into from
Jul 19, 2021
Merged

feat(Commands): resolve mentions into objects #198

merged 11 commits into from
Jul 19, 2021

Conversation

MierenManz
Copy link
Contributor

@MierenManz MierenManz commented Jul 15, 2021

About

this should close #190
Feedback is appreciated
cc @DjDeveloperr and @grian32

Status

  • These changes have been tested against Discord API or do not contain API change.
  • This PR includes only documentation changes, no code change.
  • This PR introduces some Breaking changes.

@grian32
Copy link
Contributor

grian32 commented Jul 15, 2021

Does this also resolve ID's into objects?

@MierenManz
Copy link
Contributor Author

Does this also resolve ID's into objects?
It should do that

const mentionToRegex: MentionToRegex = {
  user: /<@!?(\d{17,19})>|(\d{17,19})/,
  role: /<@&(\d{17,19})>|(\d{17,19})/,
  channel: /<#(\d{17,19})>|(\d{17,19})/
}

These are the regex's

@MierenManz
Copy link
Contributor Author

I hate lint!

@MierenManz
Copy link
Contributor Author

@Helloyunho @grian32 and @DjDeveloperr this is now ready for review
I appreciate all feedback on the test cases. Especially because the mocking can be kinda weird.

I added a few comments to everything and I hope that explain enough.
But please if you have any questions. Just put them in the review :D

@MierenManz
Copy link
Contributor Author

Does this also resolve ID's into objects?

There was a flaw in the code where it wouldn't solve that properly. Now it does tho :D

Copy link
Contributor

@grian32 grian32 left a comment

Choose a reason for hiding this comment

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

LGTM, I've also been able to test locally with my bot and it works as expected, good job!

@MierenManz
Copy link
Contributor Author

LGTM, I've also been able to test locally with my bot and it works as expected, good job!

thanks :3 Wasn't sure if the mocking was accurate.
Seems like it is and I hope that it is good enough

Copy link
Member

@Helloyunho Helloyunho left a comment

Choose a reason for hiding this comment

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

lgtm! let's wait for dj

Copy link
Member

@DjDeveloperr DjDeveloperr left a comment

Choose a reason for hiding this comment

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

LGTM!

@DjDeveloperr DjDeveloperr merged commit 890156d into harmonyland:main Jul 19, 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.

[Feature Request] Argument parser improvements
4 participants