Improve documentation and README #82

smacker · 2017-12-12T10:25:54Z

I tried to keep it short and simple.

mcuadros · 2017-12-12T10:46:57Z

README.md

@@ -9,16 +14,22 @@ The easiest way to deploy **dashboard** is using *Docker*.
 docker run -p 8080:80 bblfsh/dashboard -bblfsh-addr <bblfsh-server-addr>


include a header to say that is the recomended way.

mcuadros · 2017-12-12T10:47:25Z

README.md

-If you don't have a bblfsh server running you can execute the dashboard and the server using the following command:
+If you don't have a bblfsh server running, please read the [getting started](https://doc.bblf.sh/user/getting-started.html) guide, to learn more about how to use and deploy a bblfsh server.
+
+If don't want to run **dashboard** using our *Docker* image you can download a binary from [releases](https://github.com/bblfsh/dashboard/releases) page and run it as


make a header, to clarify that this is not the recomended way, or just simple remove this paragraph.

mcuadros · 2017-12-12T10:48:11Z

README.md

 ```

-Please read the [getting started](https://doc.bblf.sh/user/getting-started.html) guide, to learn more about how to use and deploy a bblfsh server.
+## Architecture


This is not relevant for user, only for contributors, remove from here and place in a CONTRIBUTING.md file.

mcuadros · 2017-12-12T10:48:32Z

README.md

@@ -41,6 +52,18 @@ make serve

 And access the dashboard through http://localhost:9999

+### Hot reloading mode


Not relevant for the user, remove from here.

mcuadros · 2017-12-12T10:49:31Z

README.md

+
+It's user-friendly tool for testing and research how babelfish parse code.
+
+![Screenshot](docs/screenshot.png?raw=true)


move the image to a images folder or something

smacker · 2017-12-12T11:27:15Z

I moved all developer-related information to CONTRIBUTING.md. Changed directory name for images. Added necessary headers.

mcuadros · 2017-12-12T11:43:42Z

CONTRIBUTING.md


-Contributions require sign-off. The sign-off is a simple line at the end  of the commit message for the patch or pull request, which certifies that you wrote it or otherwise have the right to pass it on as an open-source patch. The rules are pretty simple: if you can certify the below:
+source{d} go-git project is [GPLv3](LICENSE) and accept


haha. thanks!!!

mcuadros · 2017-12-12T11:44:53Z

README.md

 ```

-### Access the dashboard locally
+### Not recommended


Just say standalone or something, is ok if the people want to used :D

mcuadros · 2017-12-12T11:45:27Z

README.md

 ```sh
-make assets
+docker run -p 8080:80 bblfsh/dashboard -bblfsh-addr <bblfsh-server-addr>


Maybe we can explain a little bit more, about, the por being opened, etc.

bzz

Very nice!

One minor thing - can we please link CONTRIBUTING.md from the README i.e by having a development section, same way as we point out location of the LICENSE file?

This would remove unnecessary guesswork for the first-time contributors.

smacker · 2017-12-12T14:17:03Z

Updated.

dpordomingo

Great! At least we have more info on how to launch the dashboard! Good work;
I'd consider the following

dpordomingo · 2017-12-12T16:22:47Z

README.md


 ```sh
-docker run --privileged -d -p 9432:9432 --name bblfsh bblfsh/server
-docker run -p 8080:80 --link bblfsh bblfsh/dashboard -bblfsh-addr bblfsh:9432


I don't think it's a good idea to hide this info. People might find useful to launch the dashboard without knowing about docker or bblfsh, and these 2 lines solve it without extra effort on our side.
(It may be moved to the CONTRIBUTING#development section)

I just did what Maximo said:
#66 (comment)

dpordomingo · 2017-12-12T16:36:26Z

CONTRIBUTING.md

+
+The dashboard contains 2 parts:
+
+- Single page react application


I'd add:
bootstrapped with [Create React App](https://github.com/facebookincubator/create-react-app)
so people will be able to dig a bit more into its architecture.

not sure it worth to mention, but added

dpordomingo · 2017-12-12T16:46:39Z

CONTRIBUTING.md

+- Single page react application
+- Go web server to proxy and transform requests to bblfsd server
+
+For production usage we embed static javascript into go server using [go-bindata](https://github.com/jteeuwen/go-bindata).


Not only js code; I'd reword to something like:

For production usage, all static files are served from the go server after being embed in the server itself using go-bindata.

dpordomingo · 2017-12-12T16:55:32Z

CONTRIBUTING.md

+```sh
+make assets
+```
+


I'd add how to run the tests:

CI=true yarn test` go test ./server/...

by the way why CI=true? usually CI env variable means to change output format (from human readable to CI readable), but in our case there is no difference.

added sections about running tests

the default format when CI is not set is "interactive" what is a pain for me. But ok, lets the user to decide about it

dpordomingo · 2017-12-12T17:03:16Z

CONTRIBUTING.md

+
+Dashboard will be available on http://localhost:3000
+
+But it still requires go server on `9999` port to be available.


To do so it can be run:

go install github.com/bblfsh/dashboard/server/... bblfsh-dashboard -bblfsh-addr=0.0.0.0:9432

better go run server/cmd/bblfsh-dashboard/main.go, not sure it worth to mention here though. make serve mentioned before works just fine.

I agree with the go run instead of the go install + run it. Way simpler.

About if it worth to mention it, I think it does. In this section, it is said how to run (for development purposes) the dashboard, and it is warned that the server is needed if it's proceded that way.
I think it worth to mention it to let the user launch the front and the back separately and explore its internals. Using make serve for this purpose is more confused because it does things not really needed for this goal and way slower by the same reason.

dpordomingo · 2017-12-12T17:06:12Z

README.md


-And access the dashboard through http://localhost:9999
+Please take a look at [CONTRIBUTING](CONTRIBUTING.md) file to see how to contribute in this project.


I'd add something like:

and get more info about the dashboard architecture and how to launch it for development purposes

With links to that sections.

dpordomingo · 2017-12-13T07:39:12Z

Great, thanks,
I'd require only this one #82 (comment)

smacker · 2017-12-13T07:57:23Z

Added.

dpordomingo

LGTM
Since all comments has been already addressed, you can now squash and merge 💃

Fix #37 Signed-off-by: smacker <max@smacker.ru>

smacker requested review from dpordomingo and bzz December 12, 2017 10:25

mcuadros requested changes Dec 12, 2017

View reviewed changes

mcuadros reviewed Dec 12, 2017

View reviewed changes

mcuadros requested changes Dec 12, 2017

View reviewed changes

bzz mentioned this pull request Dec 12, 2017

Doc: update development instructions #84

Merged

bzz requested changes Dec 12, 2017

View reviewed changes

dpordomingo requested changes Dec 12, 2017

View reviewed changes

dpordomingo approved these changes Dec 13, 2017

View reviewed changes

Improve documentation and README

bd17c3c

Fix #37 Signed-off-by: smacker <max@smacker.ru>

smacker merged commit ff71d5a into bblfsh:master Dec 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve documentation and README #82

Improve documentation and README #82

smacker commented Dec 12, 2017

mcuadros Dec 12, 2017

mcuadros Dec 12, 2017

mcuadros Dec 12, 2017

mcuadros Dec 12, 2017

mcuadros Dec 12, 2017

smacker commented Dec 12, 2017

mcuadros Dec 12, 2017

smacker Dec 12, 2017

mcuadros Dec 12, 2017

mcuadros Dec 12, 2017

bzz left a comment

smacker commented Dec 12, 2017

dpordomingo left a comment

dpordomingo Dec 12, 2017

smacker Dec 13, 2017

dpordomingo Dec 12, 2017

smacker Dec 13, 2017

dpordomingo Dec 12, 2017

smacker Dec 13, 2017

dpordomingo Dec 12, 2017

smacker Dec 13, 2017

smacker Dec 13, 2017

dpordomingo Dec 13, 2017

dpordomingo Dec 12, 2017

smacker Dec 13, 2017 •

edited

Loading

dpordomingo Dec 13, 2017 •

edited

Loading

dpordomingo Dec 12, 2017

smacker Dec 13, 2017

dpordomingo commented Dec 13, 2017

smacker commented Dec 13, 2017

dpordomingo left a comment

		@@ -9,16 +14,22 @@ The easiest way to deploy dashboard is using Docker.
		docker run -p 8080:80 bblfsh/dashboard -bblfsh-addr <bblfsh-server-addr>

		@@ -41,6 +52,18 @@ make serve

		And access the dashboard through http://localhost:9999

		### Hot reloading mode


		It's user-friendly tool for testing and research how babelfish parse code.

		![Screenshot](docs/screenshot.png?raw=true)


		Contributions require sign-off. The sign-off is a simple line at the end of the commit message for the patch or pull request, which certifies that you wrote it or otherwise have the right to pass it on as an open-source patch. The rules are pretty simple: if you can certify the below:
		source{d} go-git project is [GPLv3](LICENSE) and accept


		The dashboard contains 2 parts:

		- Single page react application


		Dashboard will be available on http://localhost:3000

		But it still requires go server on `9999` port to be available.


		And access the dashboard through http://localhost:9999
		Please take a look at [CONTRIBUTING](CONTRIBUTING.md) file to see how to contribute in this project.

Improve documentation and README #82

Improve documentation and README #82

Conversation

smacker commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacker commented Dec 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzz left a comment

Choose a reason for hiding this comment

smacker commented Dec 12, 2017

dpordomingo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smacker Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

dpordomingo Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpordomingo commented Dec 13, 2017

smacker commented Dec 13, 2017

dpordomingo left a comment

Choose a reason for hiding this comment

smacker Dec 13, 2017 •

edited

Loading

dpordomingo Dec 13, 2017 •

edited

Loading