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

[enhancement/readme] Improved readme #48

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Alathreon
Copy link
Contributor

Pull-request

Changes

  • Existing code
  • New feature

Closes Issue: #44

Description

Replace this sentence with general description of what your Pull Request does.

@Alathreon Alathreon requested review from a team as code owners August 1, 2024 22:01
@Alathreon Alathreon linked an issue Aug 1, 2024 that may be closed by this pull request
@@ -2,14 +2,39 @@
## Prerequisites
- Java 21
- Docker
## Used technologies
Copy link
Member

Choose a reason for hiding this comment

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

  • Rename to "Technology Stack".
  • Merge this section with Prerequisites section;
  • Add a note in which you inform that Docker is mandatory.
  • Add version number of each technology used.


# Structure of the project
There are two projects, JShellAPI and JShellWrapper
JShellAPI is a REST API, and whenever some code is received, it will create a session, by creating a docker container, which will run JShellWrapper inside and then execute the given code in JShellWrapper.
There are two projects, JShellAPI and JShellWrapper.
Copy link
Member

Choose a reason for hiding this comment

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

  • It would be nice to make JShellAPI and JShellWrapper words as links.
  • it's better to say This project is composed of 2 sub-projects, both are Backend services (as indicated in the name of the repo)
  • I highly suggest to share the source of the diagram so we can update anytime we want.

JShellAPI is a REST API, and whenever some code is received, it will create a session, by creating a docker container, which will run JShellWrapper inside and then execute the given code in JShellWrapper.
There are two projects, JShellAPI and JShellWrapper.

JShellAPI is a REST API, where whenever some code is received, it will create a session, by creating a docker container, which will run JShellWrapper inside and then execute the given code.
Copy link
Member

Choose a reason for hiding this comment

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

  • Add a title here to inform that this section is about the 1st sub-project;
  • Bring any relevant information to JShellApi project under this section;
  • It would be better to make REST API keyword as a link to Restful API website;
  • We need to explain to explain that REST API (JShellApi) is the entry point to JShellWrapper and not the opposite.
  • We need to explain why we don't allow executing the code within the REST API service (reasons or links);


JShellAPI is a REST API, where whenever some code is received, it will create a session, by creating a docker container, which will run JShellWrapper inside and then execute the given code.

![JShellAPI diagram](https://github.com/user-attachments/assets/c566b8c0-cf81-4a56-8a0a-5715c33105a7)
Copy link
Member

Choose a reason for hiding this comment

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

  • Why not putting the diagram img within the repo ?
  • There is no mention of a scenario for using an existing session;
  • Why there is no mention to session on the diagram?


![JShellAPI diagram](https://github.com/user-attachments/assets/c566b8c0-cf81-4a56-8a0a-5715c33105a7)

There are three unrelated ways to run JShellPlaygroundBackend:
Copy link
Member

Choose a reason for hiding this comment

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

it would be better to say There are 3 ...

* * On `Windows Powershell`, use `$env:evalTimeoutSeconds=15 && $env:sysOutCharLimit=1024`
* * On `Linux Shell`, use `export evalTimeoutSeconds=15 && export sysOutCharLimit=1024`
* Run `./gradlew JShellWrapper:run`
* Alternatively, run:
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to delete this part :

* Set the environment variable `evalTimeoutSeconds=15` and `sysOutCharLimit=1024`
* * On `Windows CMD`, use `set evalTimeoutSeconds=15 && set sysOutCharLimit=1024`
* * On `Windows Powershell`, use `$env:evalTimeoutSeconds=15 && $env:sysOutCharLimit=1024`
* * On `Linux Shell`, use `export evalTimeoutSeconds=15 && export sysOutCharLimit=1024`
* Run `./gradlew JShellWrapper:run`
* Alternatively, run:

curl --request POST --url http://localhost:8080/jshell/eval/test --data '2+2'
```

### How to use JShellApi ?
Copy link
Member

Choose a reason for hiding this comment

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

Please place this under a JShellAPi section

curl --request POST --url http://localhost:8080/jshell/eval/test --data '2+2'
```

### How to use JShellApi ?
See [JShellAPI README](JShellAPI/README.MD)
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to bring the content here and add note to inform that it's NOT updated

@@ -45,5 +70,12 @@ JShellAPI is a REST API, and whenever some code is received, it will create a se
-application.yaml
```

## How to use JShellApi ?
### Try JShell API
Copy link
Member

Choose a reason for hiding this comment

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

Place this section under JShellApi section

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.

Improve readme
2 participants