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

Tooling improvements #107

Open
IanConnolly opened this issue Dec 4, 2018 · 1 comment
Open

Tooling improvements #107

IanConnolly opened this issue Dec 4, 2018 · 1 comment

Comments

@IanConnolly
Copy link
Contributor

Hey!

I've (and @pbiggar too ;)) been having some difficulties when trying to contribute some of our forked changes upstream. We have a fork (darklang/bucklescript-tea) with changes that we're baking before PR'ing here, or workarounds for issues we file, as well as some wrapped fixes in the app that also have been recently upstreamed. However, the process of going from (fork|our app)->this repo has a couple of footguns along the way that we'd like to help remove.

  1. There's no package-lock.json in the repo, so if you npm install on a clean clone it will grab bucklescript 4.07. That'll lead to a bunch of dirty files in lib/ when you run the build
  2. bin/dual-syntax is not idempotent, running it twice (say, having cancelled a commit due to missing a file) will basically give you refmt'd OCaml and Reason. In general I don't understand why both are in tree, but it seems as if the standard seems to be working from the OCaml (as it is hand-formatted) so blowing it a way is less than ideal! We could either improve the script or also add OCamlformat to the repo to ensure the OCaml stays better-formatted than what refmt's OCaml mode outputs.
  3. It's unclear when generated files should be included in PRs, which along with point 1) leads to some confusion on my part. Do these need to be in tree? If so I'd appreciate adding some guidelines about when to include them in a PR
  4. It's difficult to be sure if an API has changed due to lack of mlis/type signatures. I don't have a strong preference for having them (though I think mlis would be good from a documentation POV), but I think it'd be good to have some automation for comparing the inferred mlis at the least?

Happy to help out with the work here if you point me in the right direction :)

@OvermindDL1
Copy link
Owner

There's no package-lock.json in the repo, so if you npm install on a clean clone it will grab bucklescript 4.07. That'll lead to a bunch of dirty files in lib/ when you run the build

True true... Fixed in 852c74f (and updated bucklescript while I was at it). :-)

bin/dual-syntax is not idempotent, running it twice (say, having cancelled a commit due to missing a file) will basically give you refmt'd OCaml and Reason. In general I don't understand why both are in tree, but it seems as if the standard seems to be working from the OCaml (as it is hand-formatted) so blowing it a way is less than ideal! We could either improve the script or also add OCamlformat to the repo to ensure the OCaml stays better-formatted than what refmt's OCaml mode outputs.

It was committed by someone to make it easier to read for ReasonML users. The OCaml side is definitely the main one, and yeah refmt does change on occasion.

I've never used ocamlformat (didn't know it existed, always used ocaml-indent), that could be useful... refmt's ocaml output is readable most of the time and horrifying some other times. ^.^;

I'd definitely be up for formatting the OCaml if it works well, a lot of it is rather specifically formatted for ease of my sanity (though, honestly, in those areas those should probably be broken up in to more functions anyway...).

It's unclear when generated files should be included in PRs, which along with point 1) leads to some confusion on my part. Do these need to be in tree? If so I'd appreciate adding some guidelines about when to include them in a PR

They are there so a javascript library can use them directly without needing bucklescript installed at all. You don't really need to worry about including them in a PR as I'll generate them on a release, however it is useful to generate them in a PR to see if there are some odd thunks that popped up or something. I mostly just use their appearance in a PR as a sign that someone ran at least npm run build. :-)

It's difficult to be sure if an API has changed due to lack of mlis/type signatures. I don't have a strong preference for having them (though I think mlis would be good from a documentation POV), but I think it'd be good to have some automation for comparing the inferred mlis at the least?

Yeah I've been leaning to adding in mli's lately, I usually don't generate them until I'm decently into a project and can see what needs to be public and what shouldn't (though in general in this project everything is mostly designed to be 'public' for ease of overriding and so forth). If you want to generate mli's for all the files I'd be up to accept that PR. :-)

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

No branches or pull requests

2 participants