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: Minor parameter catching #463

Merged
merged 14 commits into from Feb 9, 2022
Merged

refactor: Minor parameter catching #463

merged 14 commits into from Feb 9, 2022

Conversation

V3ntus
Copy link
Contributor

@V3ntus V3ntus commented Feb 3, 2022

About

  • 001c23a : raise InteractionException if required parameter "description" is missing in Option's
  • 423542e : raise InteractionException if parameter "name" contains uppercase characters
  • 33f05c6 : raise InteractionException if
    1. command name is longer than 32 characters
    2. command description is longer than 100 characters
    3. more than 25 options in command
    4. autocomplete is set to true, but choices are present in command decorator
    5. command option name is longer than 32 characters
    6. command option description is longer than 100 characters

Checklist

  • I've ran pre-commit to format and lint the change(s) made.
  • I've checked to make sure the change(s) work on 3.8.6 and higher.
  • This fixes/solves an Issue.
    • (If existent): If users do not supply required parameters in command creation, no error handling would take place, and (example) a TypeError will occur:
    ...
    File "/usr/local/lib/python3.8/dist-packages/interactions/client.py", line 229, in _synchronize
      await self.__create_sync(payload)
    File "/usr/local/lib/python3.8/dist-packages/interactions/client.py", line 144, in __create_sync
      command: ApplicationCommand = ApplicationCommand(
    TypeError: type object argument after ** must be a mapping, not NoneType
  • I've made this pull request for/as: (check all that apply)
    • Documentation
    • Breaking change
    • New feature/enhancement
    • Bugfix

interactions/client.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@V3ntus V3ntus left a comment

Choose a reason for hiding this comment

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

Should an exception be raised for uppercase names, or should we convert to lower case and pass?

interactions/client.py Outdated Show resolved Hide resolved
Co-authored-by: EdVraz <88881326+EdVraz@users.noreply.github.com>
@EdVraz
Copy link
Contributor

EdVraz commented Feb 3, 2022

Should an exception be raised for uppercase names, or should we convert to lower case and pass?

I would raise an exception because it is a bad practice when you are doing it the wrong way and we just pass it the way that it should be

@EdVraz
Copy link
Contributor

EdVraz commented Feb 3, 2022

Also, please run pre-commit

@V3ntus V3ntus marked this pull request as ready for review February 3, 2022 18:13
@V3ntus
Copy link
Contributor Author

V3ntus commented Feb 3, 2022

Pre-commit struggling server-side today
10dbeb2 passes pre-commit local-side

@EdVraz
Copy link
Contributor

EdVraz commented Feb 3, 2022

Pre-commit struggling server-side today 10dbeb2 passes pre-commit local-side

the ubuntu check passes so it should be fine

Copy link
Contributor

@EdVraz EdVraz left a comment

Choose a reason for hiding this comment

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

LGTM

@V3ntus V3ntus requested a review from EdVraz February 6, 2022 21:34
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
interactions/client.py Outdated Show resolved Hide resolved
@EdVraz EdVraz changed the title Minor parameter catching refactor: Minor parameter catching Feb 6, 2022
@EdVraz EdVraz requested review from EdVraz and FayeDel February 6, 2022 23:20
Copy link
Contributor

@i0bs i0bs left a comment

Choose a reason for hiding this comment

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

Loving the appropriated use of the exception here. :)

Please run pre-commit one last time.

@V3ntus
Copy link
Contributor Author

V3ntus commented Feb 7, 2022

pre-commit returned all green, nothing to commit

@EdVraz EdVraz merged commit e820e7e into interactions-py:unstable Feb 9, 2022
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.

None yet

3 participants