Eval cog improvements #710
Merged
Merged
Conversation
It allows the cog to also work on Windows, because of Signals.SIGKILL not being defined on this platform
Akarys42
added
a: frontend
It could lead to a misleading result if it is stripped.
If the eval message is edited after less than 10 seconds, an emoji is added to the message, and if the user adds the same, the snippet is re-evaluated. This make easier to correct snipper mistakes.
The use of lambdas made the functions hard to test, this new format allows us to easily test those functions and document them.
…xt managers It can be used to test aiohttp request functions, since they are async context managers
Contributor
Author
|
5 tasks done out of 5, it is now ready for reviewing! |
Akarys42
requested review from
ikuyarihS and
jerbob
and removed request for
a team
Akarys42
added
p: 1 - high
Contributor
|
At the moment, after calling
|
ikuyarihS
previously requested changes
Feb 10, 2020
MarkKoz
requested changes
Feb 15, 2020
This avoid recreating partials for each re-eval
It was triggering an error if the user deleted the output before re-evaluating
Makes the code a bit clearer Co-authored-by: Shirayuki Nekomata <lethienphuoc1991@gmail.com>
Co-Authored-By: Mark <kozlovmark@gmail.com>
Reads better as separate lines Co-Authored-By: Mark <kozlovmark@gmail.com>
Reduce visual clutter Co-Authored-By: Mark <kozlovmark@gmail.com>
Because of the stripping, it should still be considered as empty Co-Authored-By: Mark <kozlovmark@gmail.com>
MarkKoz
added
s: waiting for author
lemonsaurus
approved these changes
Feb 23, 2020
Contributor
lemonsaurus
left a comment
lemonsaurus
left a comment
There was a problem hiding this comment.
Overall I think this is quite a good pull request, mostly with clean and atomic commits and making some nice improvements. The re-eval feature, which you even wrote the issue for, is particularly well implemented.
Nice work.
Two functions were created: send_eval and continue_eval, in order to facilitate testing. The corresponding tests are also changed in this commit.
Unicode literals aren't really safe compared to code points
Akarys42
removed
the
s: waiting for author
MarkKoz
requested changes
Feb 27, 2020
Contributor
MarkKoz
left a comment
MarkKoz
left a comment
There was a problem hiding this comment.
Thanks. The split make it look much nicer. Just several more small things.
It could have caused some errors if the user delete his own message
MarkKoz
approved these changes
Feb 28, 2020
Contributor
MarkKoz
left a comment
MarkKoz
left a comment
There was a problem hiding this comment.
This is great work. Thanks for taking the initiative to fix those bugs and, of course, add the very useful re-evaluation feature. It's a simple and intuitive design. I believe it will be much appreciate by the users (I made a change to hopefully help them know about it). 100% coverage on the test suite is impressive - I know how tedious achieving that can be, especially for larger features such as snekbox.
MarkKoz
dismissed
ikuyarihS’s stale review
Author has addressed the requested changes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Now that snekbox support some external libraries (see python-discord/snekbox#51), I think this is a good time to work on some improvements :
Signals.SIGKILLnot being definedbot/cogs/snekbox.py(closes Write unit tests forbot/cogs/snekbox.py#588)