Closed yaacov closed 10 months ago
@nirs @sleviim hi, can you review / refer to a reviewer ?
I don't know much about modern javascript, but I don't have time to maintain this code. If you want to maintain this code and this change makes it easier for you than why not.
Thanks, sure I can help maintain the code ( part time :-) ), @sleviim do you know who currently maintain this code, do you want me to help ?
Thanks, @yaacov I also don't have time to maintain the code. I know that @eshulman2 had some code contributions @nirs can we also add her as a reviewer?
I don't know much about modern javascript, but I don't have time to maintain this code. If you want to maintain this code and this change makes it easier for you than why not.
I will be happy to help maintain the code, here is some reference for this PR:
in the last session of Nitazanim the students where given a task to write a pull request for the ROSE repository, the students had some trouble relate to the code.
The ROSE code base uses deprecated dialects: a. python 2.7 is deprecated (since 2020), we should teach student to use python3 b. ES5 is depracated (since 2015), we should teach students ES6
The ROSE code uses refactored logic, students may need more experience to understand: a. for example score calculating logic is refactored to use reverse tests.
This PR is for making the code more ready for next year Nitzanim task, but IMHO more work need to be done to update ROSE code, for example: a. replace Twisted with asyncio for python3 compatibility. b. replace RPC communication with HTTP request response logic for better compatibility with Openshift stateless containers. c. ML examples, or at least some docs on how to train an a driver using pytorch, tensorflow or keras
Thanks for the notes @yaacov ! Currently, we're running the code via virtual env, cause running the code locally raised some deps issues, as the twisted library (see https://github.com/RedHat-Israel/ROSE/pull/454) I guess it won't affect this one? @nirs wdyt? Is it better to keep using the venv?
about #454 If we are moving to containers and Openshift we can replace both venv and pipenv with podman build
Thanks, @yaacov I also don't have time to maintain the code. I know that @eshulman2 had some code contributions @nirs can we also add her as a reviewer?
Sure, please do.
@yaacov @sleviim an issue is a better place to discuss replacing the venv with a container.
@cben hi, I am looking for a reviewer, can you review this PR ?
@sgratch hi, I am looking for a reviewer, can you review this PR ?
@bennypowers thanks for the review :heart:
I have updated the PR according to comment https://github.com/RedHat-Israel/ROSE/pull/475#discussion_r1315389310
this looks like a perfect example of a web components use case. Instead of creating a Game class and having it "mount" on some div in the DOM, what about having something like this:
Sounds cool ! it will be great idea for the next PR.
@sleviim @nirs hi, @bennypowers reviewed the code, it's as reviewed as we can get, and ready for merge.
Note: The web-component upgrade is an improvement we think will make the code simpler and easy for students to understand, but it's a big change, not in the scope of this PR, @bennypowers and me talked about it offline, and we will look at the best way to implement web components in later time.
Issue: Current Javascript user interface uses very old Javascript syntax and depend on jQuery, moving to ES6 (also known as ECMAScript 2015) and removing jQuery from a project can have several benefits:
What are the updates proposed by this PR:
var
withlet
orconst
based on mutability.class
syntax.jQuery
AJAX withFetch
: TheFetch
API is a modern way to make network requests and can replace jQuery's$.ajax
or$.post
.document.querySelector()
ordocument.querySelectorAll()
instead of$()
.addEventListener
instead of jQuery's.click()
, .on()
, etc.