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

Clarify usage section in README.md #3092

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Conversation

postmeback
Copy link
Contributor

There were certain installation steps missing in the ReadMe file, which I added in order to get it corrected.

Added missing steps, which I faced while installing it.
@jedel1043
Copy link
Member

IIRC cargo run -- test is equivalent to cargo build & cargo run -- test, so I don't see why this should fail. Do you have an error log that we could look at?

@postmeback
Copy link
Contributor Author

postmeback commented Jul 1, 2023

I restarted my system, and executed "cargo run -- Hello2.js", it ran successfully. But after installation, when I was trying to do so, it was failing. Sorry, I don't have any screenshots of that.

2023-07-01

Today's success.

README.md Outdated
@@ -64,7 +64,8 @@ then go to `http://localhost:8080`.
## Usage

- Clone this repo.
- Run with `cargo run -- test.js` where `test.js` is an existing JS file with any JS valid code.
- Run `cargo build`
- Run with `cargo run -- test.js` where `test.js` is an existing JS file with any JS valid code. The file has to be in the root directory.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gives the person a clear idea to where the file should be added; it's worth mentioning.

Copy link
Member

Choose a reason for hiding this comment

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

It gives the person a clear idea to where the file should be added; it's worth mentioning.

It doesn't have to be in the root dir, it just require a path to the file. so cargo run -- ../some-folder-outside-root/works.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HalidOdat , you are correct. But if somebody changes the directory to the target folder, and then runs cargo-run, then it will fail. I think, for better clarity, we should be mentioning it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although, alternative suggestions are always welcome

Copy link
Member

Choose a reason for hiding this comment

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

@HalidOdat , you are correct. But if somebody changes the directory to the target folder, and then runs cargo-run, then it will fail. I think, for better clarity, we should be mentioning it.

This, should be obvious, cargo doesn't work if Cargo.toml is not found (applies to pretty much every build system)

Although, alternative suggestions are always welcome

We can make it explicit:

"Run with cargo run -- test.js in the project root directory where test.js is a path to an existing JS file with any JS valid code."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HalidOdat I would request you to put suggestions in this PR. That would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

It may be a bit more succinct to write.

 - Run with `cargo run -- test.js` in the root directory where `test.js` is a path to any existing valid JS file.

Do we need to double reference JS file and valid JS code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Double reference is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be a bit more succinct to write.

 - Run with `cargo run -- test.js` in the root directory where `test.js` is a path to any existing valid JS file.

Do we need to double reference JS file and valid JS code?

It looks perfect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, I have updated the steps as per the discussion. requesting for review and merge.

README.md Outdated
@@ -64,7 +64,8 @@ then go to `http://localhost:8080`.
## Usage

- Clone this repo.
- Run with `cargo run -- test.js` where `test.js` is an existing JS file with any JS valid code.
- Run `cargo build`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the first time, to be on the safer side, I believe we should mention this step.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is needed, cargo should always build if you run (and it is not already built). If it doesn't, that means it is a bug in cargo and should be reported in the cargo repo https://github.com/rust-lang/cargo/

Copy link
Member

Choose a reason for hiding this comment

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

Note that we should still remove the cargo build part, since it's not needed and is adds complexity to the steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that part.

Updated steps as per discussion
Removed `cargo build` from the steps
README.md Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #3092 (72f8314) into main (ce520fa) will decrease coverage by 0.02%.
Report is 78 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3092      +/-   ##
==========================================
- Coverage   50.42%   50.40%   -0.02%     
==========================================
  Files         444      436       -8     
  Lines       45877    42375    -3502     
==========================================
- Hits        23132    21358    -1774     
+ Misses      22745    21017    -1728     

see 228 files with indirect coverage changes

Textual Changes
@postmeback
Copy link
Contributor Author

Hey, can this be merged ?

@HalidOdat HalidOdat changed the title Update README.md || Added missing steps, which I faced while installing it. Clarify usage section in README.md Aug 1, 2023
@HalidOdat HalidOdat added enhancement New feature or request documentation update documentation labels Aug 1, 2023
@HalidOdat HalidOdat added this to the v0.18.0 milestone Aug 1, 2023
@HalidOdat HalidOdat enabled auto-merge August 1, 2023 14:56
@HalidOdat HalidOdat added this pull request to the merge queue Aug 1, 2023
Merged via the queue into boa-dev:main with commit 0769260 Aug 1, 2023
postmeback added a commit to postmeback/boa that referenced this pull request Aug 1, 2023
@postmeback postmeback deleted the patch-1 branch August 1, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation update documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants