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

Update and simplify README #591

Merged
merged 1 commit into from Aug 3, 2016
Merged

Update and simplify README #591

merged 1 commit into from Aug 3, 2016

Conversation

ghost
Copy link

@ghost ghost commented Aug 3, 2016

This PR updates and simplifies the README. In particular, I've updated the build instructions to just mention npm install. I've removed some extra commentary in the README that I think might be better suited to the wiki or other docs. If you find this PR acceptable I could help add the commentary (licensing, status) to the wiki and include links in the README.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling 0d7edb5 on freebroccolo:update-readme into 0212ef6 on bloomberg:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling 0d7edb5 on freebroccolo:update-readme into 0212ef6 on bloomberg:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling 57ca44b on freebroccolo:update-readme into 0212ef6 on bloomberg:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling 23fc796 on freebroccolo:update-readme into 0212ef6 on bloomberg:master.


## Disclaimer
* Standard C/C++ toolchain
* Node 4 or newer
Copy link
Member

Choose a reason for hiding this comment

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

node is not a dependency, if people build from source, and we don't depend on c++ toolchain

Copy link
Author

Choose a reason for hiding this comment

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

Good points. The reason I put Node there is because it comes with npm but I could clarify that (when installing the package) and fix the other one.

@bobzhang
Copy link
Member

bobzhang commented Aug 3, 2016

I like the new README in general, it looks more professional, sorry for my bad English :-)
Some comments posted

@ghost
Copy link
Author

ghost commented Aug 3, 2016

Thanks. Regarding the licensing and building from source sections, what do you think about moving them to a separate markdown file or a wiki page and just have a very clear link under the section header?

I think it's great to make the extra information available but maybe not include all of it directly in the main README. I worry that seeing all the detailed build instructions right away could be intimidating to users who don't plan to work on the source.

@bobzhang
Copy link
Member

bobzhang commented Aug 3, 2016

I agree, would you mind have a file in docs about building instructions?
Let's keep the Licensing section here, it needs more careful review to move such stuff, thanks!

@ghost
Copy link
Author

ghost commented Aug 3, 2016

@bobzhang I updated the PR based on your feedback. The build instructions are in an external file and the license section is added back in (I tried to restructure some of the links and stuff to make it a bit clearer but can revert that part if you prefer).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling 0753f79 on freebroccolo:update-readme into 9c327d6 on bloomberg:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling e0fb4b6 on freebroccolo:update-readme into 9c327d6 on bloomberg:master.

@bobzhang
Copy link
Member

bobzhang commented Aug 3, 2016

Thanks, looks much better now. Except for the explanation of bs attributes, with bs attribute, the compiler would guarantee to generate code following uncurried calling convention (required by FFI from JS side), without bs attribute, the compiler has freedom to optimize.

@ghost
Copy link
Author

ghost commented Aug 3, 2016

Okay, does that part look better now?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling 2efb762 on freebroccolo:update-readme into 9c327d6 on bloomberg:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 50.731% when pulling 2efb762 on freebroccolo:update-readme into 9c327d6 on bloomberg:master.

@bobzhang
Copy link
Member

bobzhang commented Aug 3, 2016

thanks!

@bobzhang bobzhang merged commit 54d4571 into rescript-lang:master Aug 3, 2016
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.

None yet

3 participants