-
Notifications
You must be signed in to change notification settings - Fork 223
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
README.md: Modify steps of Usage #700
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modifications are good.Just suggesting a minor change in the commit body.
This modifies the steps required for the
usage of the repository .
Replacing "usage of the repository" with building project website or something similar to it would make more sense according to me.
@budukhyash plz review. |
Travis tests have failedHey @sanchit48, Ruby: 2.5.1docker run -v=$(pwd):/app --workdir=/app coala/base coala --ci
TravisBuddy Request Identifier: 62363210-2150-11e9-af85-47c84d506d52 |
README.md
Outdated
@@ -29,9 +29,13 @@ Why? | |||
|
|||
## Usage | |||
|
|||
To clone the repository and run this website on your local machine, [install Jekyll](https://jekyllrb.com/docs/installation/) for your OS and type the following commands: | |||
To clone the repository and run this website on your local machine, install | |||
[Jekyll](https://jekyllrb.com/docs/installation/) and [rbenv](https://www.digitalocean.comcommunity/tutorials/how-to-install-ruby-on-rails) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to link an external installation guide to install anything. The official installation guide for rbenv should be linked here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frextrite I actually can't find the official installation guide for rbnev but i did find it's github repo. plz check if it's ok https://github.com/rbenv/rbenv#basic-github-checkout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work. Please consult other reviewers as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@li-boxuan @utkarsh2102 @shashank-b @akshatkarani plz review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are doing this, lets switch to @asdf-vm , which wraps rbenv and others. And they have a moderately sane website at https://asdf-vm.github.io/asdf/#/ . Iit should work ok on windows even, and ive added asdf-vm/asdf#450 to add CI for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
This comment has been minimized.
This comment has been minimized.
README.md
Outdated
@@ -29,9 +29,13 @@ Why? | |||
|
|||
## Usage | |||
|
|||
To clone the repository and run this website on your local machine, [install Jekyll](https://jekyllrb.com/docs/installation/) for your OS and type the following commands: | |||
To clone the repository and run this website on your local machine, install | |||
[Jekyll](https://jekyllrb.com/docs/installation/) and [rbenv](https://www.digitalocean.comcommunity/tutorials/how-to-install-ruby-on-rails) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are doing this, lets switch to @asdf-vm , which wraps rbenv and others. And they have a moderately sane website at https://asdf-vm.github.io/asdf/#/ . Iit should work ok on windows even, and ive added asdf-vm/asdf#450 to add CI for that.
Travis tests have failedHey @sanchit48, Ruby: 2.5.1docker run -v=$(pwd):/app --workdir=/app coala/base coala --ci
TravisBuddy Request Identifier: ec09d6f0-2550-11e9-8e3a-a568437420b3 |
@jayvdb plz review |
README.md
Outdated
To clone the repository and run this website on your local machine, [install Jekyll](https://jekyllrb.com/docs/installation/) for your OS and type the following commands: | ||
|
||
$ sudo gem install jekyll bundler | ||
To clone the repository and run this website on your local machine, install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the first line, creates a thought that for cloning this repository we need to install asdf-vm
. Possibly you can divide this into two sections - asdf-vm & ruby installation
and cloning and running
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KVGarg I agree with you but in the original README.md they asked to install Jekyll with the first line. So maybe they thought it won't create the confusion. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In original README.md also, it is creating a confusion too. Now, also it's ok but it is creating some confusion. Don't know about others. Aren't you getting confused ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it does creates the confusion. i will change it.
This comment has been minimized.
This comment has been minimized.
@KVGarg plz review |
README.md
Outdated
$ sudo gem install jekyll bundler | ||
Install [asdf-vm](https://asdf-vm.github.io/asdf/#/core-manage-asdf-vm) for your OS and now using asdf-vm, install [ruby](https://github.com/asdf-vm/asdf-ruby). | ||
|
||
### Cloning and running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used cloning and running
because at that time, I didn't get the right word in my mind.
Can we have something more appropriate that describe this section better in two or three words ? IDK whether this will fit to this Build and Developement
, if you have better words that can describe this section go with that. Because I am myself doubtful with my suggestion. But something diff. from Cloning and Running
Else everything perfect 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i will see :)
README.md
Outdated
|
||
$ sudo gem install jekyll bundler | ||
Install [asdf-vm](https://asdf-vm.github.io/asdf/#/core-manage-asdf-vm) for your OS and now using asdf-vm, install [ruby](https://github.com/asdf-vm/asdf-ruby). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to break them into 2 points. And since the 2nd point is dependent on the first, number them.
Dependencies:
1. asdf-vm(for installing ruby easily)
2. ruby
But this way we are forcing the developer to install ruby via asdf. I am not sure if that is a good thing or not. Personally, I agree that using scripts makes the installation easier, but these scripts tend to break a lot, and thus prefer a more extensive manual installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frextrite don't know much about that, i was just going according to #700 (comment)
README.md
Outdated
$ sudo gem install jekyll bundler | ||
Install [asdf-vm](https://asdf-vm.github.io/asdf/#/core-manage-asdf-vm) for your OS and now using asdf-vm, install [ruby](https://github.com/asdf-vm/asdf-ruby). | ||
|
||
### Cloning and running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename this to Download and Run
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frextrite but we are not downloading anything. should I use Clone and Run
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sanchit, go with Clone and Run
as clone itself refers to create a copy of repository on your local desktop by downloading it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep Clone and Run
works.
README.md
Outdated
|
||
### Cloning and running | ||
|
||
To clone the repository and run this website on your local machine, type the following commands: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, infom the user to clone the repository followed by the clone command.
Clone the coala Projects repository
git clone <link to repository>
and then the steps to run the website
Run coala Projects website
$ cmd1
$ cmd2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clone the coala Projects repository
I think To clone the repository, type the following command
would look much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will go with, frexrite. Since, both of them have same meaning but it will convey the meaning in less words in impactful manner
README.md
Outdated
|
||
To clone the repository and run this website on your local machine, type the following commands: | ||
|
||
$ gem install jekyll bundler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this somewhere before cloning the repository and after the dependency section, ideally it should be in the dependency section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you plz tell how can i do that like should i make point no. 3 and just write the command ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split dependencies section in points, this will create 3 points in the section.
- for installing asdf-vm
- for installing ruby
- for jekyll and bundler
@KVGarg plz review |
This comment has been minimized.
This comment has been minimized.
README.md
Outdated
$ sudo gem install jekyll bundler | ||
1. Install [asdf-vm](https://asdf-vm.github.io/asdf/#/core-manage-asdf-vm) for your OS. | ||
2. Using asdf-vm, install [ruby](https://github.com/asdf-vm/asdf-ruby). | ||
3. Install jekyll bundler by typing the following command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think(no idea of ruby) jekyll and bundler are 2 gems. In that case add an and
between the two and preferably bold them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frextrite plz see https://jekyllrb.com/docs/. Here the command gem install jekyll bundler
is given
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He is trying to say, that where you have writtem Install jekyll nundler
there you should write Install jekyll and bundler gems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and try to ommit by typing the following command
. It is self-explanatory to user that he has to run this command in the terminal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh ok, got it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the site Jekyll and Bundler are gems, so you can edit this line to just Install Jekyll by typing ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @KVGarg. Omitting by typing the following command
would be better.
README.md
Outdated
@@ -29,15 +29,26 @@ Why? | |||
|
|||
## Usage | |||
|
|||
To clone the repository and run this website on your local machine, [install Jekyll](https://jekyllrb.com/docs/installation/) for your OS and type the following commands: | |||
#### Dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this h3
README.md
Outdated
|
||
$ gem install jekyll bundler | ||
|
||
#### Clone and Run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as well.
README.md
Outdated
|
||
#### Clone and Run | ||
|
||
Clone the coala Projects repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the
This comment has been minimized.
This comment has been minimized.
Take a look at this and make the CI happy :) |
This comment has been minimized.
This comment has been minimized.
This modifies the steps required to build and run the website on the local machine by suggesting not to use sudo in the command to install jekyll and bundler and prefer asdf-vm to install ruby. Closes #477
This comment has been minimized.
This comment has been minimized.
Travis tests have failedHey @sanchit48, Ruby: 2.5.1docker run -v=$(pwd):/app --workdir=/app coala/base coala --ci
TravisBuddy Request Identifier: 40e8b4c0-28a8-11e9-892f-216a8a2cc0ab |
ack 0aee8f7 |
@gitmate-bot ff |
Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently |
Automated fastforward with GitMate.io was successful! 🎉 |
@jayvdb @sanchit48 CI wasn't green before getting merged due to which other PR's CI is also getting red because of failed tests over this PR. |
Closes #477