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

Jbuilder #32

Merged
merged 20 commits into from
Jun 13, 2017
Merged

Jbuilder #32

merged 20 commits into from
Jun 13, 2017

Conversation

jaredly
Copy link
Contributor

@jaredly jaredly commented Jun 13, 2017

continuation of #30

@jaredly
Copy link
Contributor Author

jaredly commented Jun 13, 2017

I think it leads to less confusion. Might has well have the directories have the same names as the compiled artifacts

@jaredly
Copy link
Contributor Author

jaredly commented Jun 13, 2017

tbh I'd rather capitalize Internal_lib and all of the source files M1.re etc.

@jordwalke
Copy link
Member

jordwalke commented Jun 13, 2017

@jaredly If you prefer it, you can capitalize the files if you like - we could also setup a case sensitive travis build to let us know if that introduces any issues.

Regardless of if the package name is reason-native-project or reason-native-project, I think when it comes to actual code, the underscores are a little less familiar to JS developers. Is there a way to allow people to consume the result of this as ReasonNativeProject.InnerModules? Or is there some limitation in jBuilder?

@jaredly
Copy link
Contributor Author

jaredly commented Jun 13, 2017

oh for sure! I was just converting from kebab to underscores, I'm totally fine with camel

@jordwalke
Copy link
Member

Nice! I'll just wait until the CI passes then merge!

@jordwalke
Copy link
Member

Happy to merge, but CI is not happy for some reason. Do you know what's up?

@jaredly
Copy link
Contributor Author

jaredly commented Jun 13, 2017

oh funny, looks like there some duplication between travis.yml and test-with-version

@jordwalke
Copy link
Member

Very good! Thank you @jaredly. Thank you @rgrinberg! This is a massive improvement.

@jordwalke jordwalke merged commit 38f925e into reasonml:master Jun 13, 2017
@kennetpostigo
Copy link
Contributor

Does the readme of the project need to be updated with the move to JBuilder?

@jordwalke
Copy link
Member

yes! Do you feel like helping to document it?
( @jaredly might have suggestions for going beyond basic instructions but feel free to send PRs for ideas )

@jaredly jaredly deleted the jbuilder branch June 17, 2017 21:36
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.

4 participants