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.

Sign up for GitHub

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

Jump to bottom

Add minmax length #1463

Merged
merged 18 commits into from
Jul 4, 2022
Merged

Add minmax length #1463

merged 18 commits into from
Jul 4, 2022

Conversation

Icebluewolf
Copy link
Contributor

Summary

Adds the min_length and max_length options to Application commands

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, typehinting, examples, ...)

Icebluewolf and others added 3 commits March 8, 2022 07:24
@Icebluewolf
Fix slash_autocomplete to not error
91572a1 
The command description docstring was to long (over 100 characters).
@Icebluewolf
Add minmax length options to slash commands
ae2e375
@Icebluewolf
Merge remote-tracking branch 'origin/master'
4b3bd6f 
# Conflicts:
#	examples/app_commands/slash_autocomplete.py
@Icebluewolf
Copy link
Contributor Author

When testing my changes I noticed that if either minmax_value or minmax_length are invalid it will not update the commands and will not throw an error.
Also if you do this Option(str, min_value=5, max_value=10) it will not register any commands and throw no error.

Im not sure if that should be fixed in a separate PR or wait to merge it until that is fixed.

@Lulalaby Lulalaby added priority: medium Medium Priority status: in progress Work in Progess feature Implements a feature Merge with squash labels Jul 3, 2022
@Lulalaby Lulalaby added this to the v2.0 milestone Jul 3, 2022
@Dorukyum Dorukyum marked this pull request as draft July 3, 2022 16:57
ArjunSharda
ArjunSharda previously approved these changes Jul 4, 2022
View reviewed changes
EmmmaTech
EmmmaTech previously approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@EmmmaTech EmmmaTech left a comment

Choose a reason for hiding this comment

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

Why is this a draft? This PR looks complete.
Anyways haven't tested but the code portions look good

@Dorukyum
Copy link
Member

Dorukyum commented Jul 4, 2022

Why is this a draft? This PR looks complete.

Waiting for value checks (0 <= min_length <= 6000, 1 <= max_length <= 6000)

@EmmmaTech
Copy link
Contributor

Why is this a draft? This PR looks complete.

Waiting for value checks (0 <= min_length <= 6000, 1 <= max_length <= 6000)

Ah okay

@Icebluewolf Icebluewolf dismissed stale reviews from EmmmaTech and ArjunSharda via 9e519cb July 4, 2022 14:30
@Lulalaby Lulalaby marked this pull request as ready for review July 4, 2022 14:37
Lulalaby
Lulalaby previously approved these changes Jul 4, 2022
View reviewed changes
@Lulalaby Lulalaby enabled auto-merge (squash) July 4, 2022 14:38
@Lulalaby
Copy link
Member

Lulalaby commented Jul 4, 2022

@plun1331 @emretech please test this for us

@Icebluewolf
Copy link
Contributor Author

I disabled auto merge because it is vital that this be tested first. I think it breaks things majorly but am not sure why yet.

Dorukyum
Dorukyum reviewed Jul 4, 2022
View reviewed changes
discord/commands/options.py Outdated Show resolved Hide resolved
discord/commands/options.py Outdated Show resolved Hide resolved
discord/commands/options.py Outdated Show resolved Hide resolved
discord/commands/options.py Outdated Show resolved Hide resolved
discord/commands/options.py Outdated Show resolved Hide resolved
@Lulalaby @Dorukyum
Apply suggestions from code review
5bb9b60 
Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
Dorukyum
Dorukyum previously approved these changes Jul 4, 2022
View reviewed changes
ArjunSharda
ArjunSharda previously approved these changes Jul 4, 2022
View reviewed changes
EmmmaTech
EmmmaTech previously approved these changes Jul 4, 2022
View reviewed changes
Copy link
Contributor

@EmmmaTech EmmmaTech left a comment

Choose a reason for hiding this comment

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

Besides this suggestion everything else is fine

discord/commands/options.py Show resolved Hide resolved
@Lulalaby Lulalaby self-requested a review July 4, 2022 19:39
@Lulalaby Lulalaby dismissed stale reviews from EmmmaTech, ArjunSharda, and Dorukyum via e74ecf7 July 4, 2022 20:34
Dorukyum
Dorukyum approved these changes Jul 4, 2022
View reviewed changes
Copy link
Member

@Dorukyum Dorukyum left a comment

Choose a reason for hiding this comment

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

Good work

@Dorukyum Dorukyum merged commit 0492613 into Pycord-Development:master Jul 4, 2022
@baronkobama baronkobama mentioned this pull request Jul 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dorukyum Dorukyum Dorukyum approved these changes

@EmmmaTech EmmmaTech EmmmaTech approved these changes

@Lulalaby Lulalaby Lulalaby approved these changes

@ArjunSharda ArjunSharda ArjunSharda left review comments

@plun1331 plun1331 Awaiting requested review from plun1331

Assignees

@Icebluewolf Icebluewolf

Labels
feature Implements a feature priority: medium Medium Priority status: in progress Work in Progess
Projects
Pycord Releases
Status: Done
Milestone
v2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Icebluewolf @Dorukyum @EmmmaTech @Lulalaby @ArjunSharda