Skip to content

Add warning message when using input with eval command#2422

Merged
wookie184 merged 4 commits into
python-discord:mainfrom
Ibrahim2750mi:snekbox/handling-input-error-message
Mar 3, 2023
Merged

Add warning message when using input with eval command#2422
wookie184 merged 4 commits into
python-discord:mainfrom
Ibrahim2750mi:snekbox/handling-input-error-message

Conversation

@Ibrahim2750mi
Copy link
Copy Markdown
Contributor

@Ibrahim2750mi Ibrahim2750mi commented Mar 1, 2023

Closes #2420
Closes (MAYBE) #2255

We could check if EOFError is in the result (as well as maybe checking that "input" is on their code and the status code indicated an error), and if so add a message after the codeblock saying that input doesn't work with the bot.

No need to check if input is in their code as the docs say EOFError is only raised when

the input() function hits an end-of-file condition (EOF) without reading any data

Screenshots:
2023-03-01_16-52_1
2023-03-01_16-52

@Ibrahim2750mi Ibrahim2750mi requested review from jb3 and ks129 as code owners March 1, 2023 11:23
@Ibrahim2750mi Ibrahim2750mi changed the title Add warning message when using input Add warning message when using input with eval command Mar 1, 2023
Comment thread bot/exts/utils/snekbox.py Outdated
@Ibrahim2750mi Ibrahim2750mi requested review from onerandomusername and wookie184 and removed request for jb3, ks129, onerandomusername and wookie184 March 1, 2023 21:18
Comment thread bot/exts/utils/snekbox.py Outdated
Ibrahim2750mi and others added 2 commits March 3, 2023 01:56

+ Double quotes for consistency
+ Using results["stdout"] instead of output so the warning is still displayed if the error doesn't fit in the message.
+ Using endswith so we don't need to hardcode the constant.

Co-authored-by: wookie184 <wookie1840@gmail.com>
@Ibrahim2750mi Ibrahim2750mi force-pushed the snekbox/handling-input-error-message branch from f99a12f to f3c1038 Compare March 2, 2023 20:28
@Ibrahim2750mi Ibrahim2750mi requested a review from wookie184 March 2, 2023 20:28
@Ibrahim2750mi
Copy link
Copy Markdown
Contributor Author

Also wookie thanks for reviewing

Comment thread bot/exts/utils/snekbox.py
Copy link
Copy Markdown
Member

@mbaruh mbaruh left a comment

Choose a reason for hiding this comment

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

A false positive here would be

!e raise ValueError("EOFError: EOF when reading a line")

Not sure this is worth handling though. If you tried to find input in the eval input you would also have a false positive in the form of someone shadowing the built-in, as beginners often do.

@Ibrahim2750mi
Copy link
Copy Markdown
Contributor Author

A false positive here would be

!e raise ValueError("EOFError: EOF when reading a line")

I also realised this and commented it in the first review there wookie recommended to leave this as no user would do this.

Not sure this is worth handling though. If you tried to find input in the eval input you would also have a false positive in the form of someone shadowing the built-in, as beginners often do.

Just doing if "input" in code: is a grave mistake same goes with raise. If we check that there is input or raise in the code we need to do it with ast and not comparing it just with string.

@Robin5605
Copy link
Copy Markdown
Contributor

Sort of along the same lines as #2255 , how feasible would it be to add a modal with a TextInput that will be fed into input()?

@Ibrahim2750mi
Copy link
Copy Markdown
Contributor Author

Ibrahim2750mi commented Mar 3, 2023

Sort of along the same lines as #2255 , how feasible would it be to add a modal with a TextInput that will be fed into input()?

I don't get whats the use of adding any input, eval command is not there to build an user interface. Any use of user input can be hard coded.

Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Works nicely, thanks!

@wookie184 wookie184 enabled auto-merge March 3, 2023 16:34
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.

Better error message for !eval when using input

5 participants