Together-Java / TJ-Bot

TJ-Bot is a Discord Bot used on the Together Java server. It is maintained by the community, anyone can contribute.
https://togetherjava.org
GNU General Public License v3.0
100 stars 87 forks source link

Feature/jshell rework #998

Closed Alathreon closed 7 months ago

marko-radosavljevic commented 9 months ago

Is this PR ready to be reviewed? Seems like a significant rework, based on the number of changes. It would probably help the review process if you gave some summary/outline of your PR.

And yeah, missing few curly boys, so sonar is mad at us: https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&sinceLeakPeriod=true&pullRequest=998&id=Together-Java_TJ-Bot :relaxed:

Thanks for your work, hopefully we will have jshell soon in prod. :heart:

Alathreon commented 9 months ago

Is this PR ready to be reviewed? Seems like a significant rework, based on the number of changes. It would probably help the review process if you gave some summary/outline of your PR.

And yeah, missing few curly boys, so sonar is mad at us: https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&sinceLeakPeriod=true&pullRequest=998&id=Together-Java_TJ-Bot ☺️

Thanks for your work, hopefully we will have jshell soon in prod. ❤️

Oops sorry for the late response, yes it is ready for a review. this PR does two things, it reworks jshell to supports the changes in the backend (support or multi snippets eval), and also big improvements of the display of the response of an eval.

sonarcloud[bot] commented 8 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Alathreon commented 8 months ago

Haven't reviewed the new stuff, but changes are sensible, mostly a change to discord effective names instead of usernames.

Can you post some UI/UX pics?

image image image

marko-radosavljevic commented 7 months ago

Generally would like to see 2 reviews for a bigger feature like this. But the PR has been sitting for quite a while, and with 1 passing review… Let's ship it, and properly test it on development server. :heart:

I skimmed over it, and it seems fine. It's modular like all our futures, so if issues arise, we can always temporarily block it. :relaxed: