Indented most of the source code #409
Conversation
Side-note: `ocp-indent` had a hard time with the syntax of this file. I manually indented part of the file.
|
AFAIAC, there are no PRs locking There are probably going to be some changes in the short term (e.g., #408) but there are no PR for these. |
|
Thanks! I'll review this in a bit. Just want to say that I'm minutes or hours away from submitting a PR for #408, but it won't touch the |
| Thread.join worker.thread | ||
| end; | ||
| Lwt.return_unit) | ||
| if worker.reuse then |
There was a problem hiding this comment.
IMO ocp-indent gets this kind of thing wrong. It shouldn't cause an extra space if you have to add a ( to a function (or any other expression). Is it easy to fix in the ocp-indent settings? If not, it's fine – no need to go through all these files. The PR does much more good than harm, even with this nit.
There was a problem hiding this comment.
Hmm, I guess it does this to align everything two spaces after the fun keyword, which is one column after the beginning of the line. I agree it looks kinda wrong. I'll see what I can do.
There was a problem hiding this comment.
Available configuration doesn't seem to provide an option for this behavior.
Is it worth opening an issue on their side?
There was a problem hiding this comment.
Sure, they might be able to answer it, fix it, or teach me/us why the ocp-indent way is the right way :) Also cc @vasilisp for any experience with this.
I don't think this blocks the PR though, unless you want to wait for it.
| in | ||
| make | ||
| ~output:(fun section level lines -> | ||
| Lwt_mutex.with_lock mutex | ||
| Lwt_mutex.with_lock mutex |
There was a problem hiding this comment.
I'm really glad things like this are getting fixed.
src/util/configure.ml
Outdated
| let f = open_out "src/jbuild-ignore" in | ||
| (if !use_camlp4 = Some(true) then () else Printf.fprintf f "camlp4"); | ||
| close_out f | ||
| let f = open_out "src/unix/lwt_config" in |
There was a problem hiding this comment.
Looks like ocp-indent got this wrong. I wonder why? I don't think this should be indented more than the previous line (am I missing something?).
There was a problem hiding this comment.
There's a similar issue reported here. I'll keep an eye on it.
There was a problem hiding this comment.
Thanks, subscribed to that issue.
|
So most of it looks good – thanks for doing this. The only other nit I have is that the glib commit should be titled something like |
|
Ok, looks great. I'm happy to have this in. I'll merge once the builds finish, but they will, of course, succeed. |
Builds on work by Richard Degenne (@Richard-Degenne) in #400 and #409. This commit indents some of the test suite. Some indentation done manually.
This PR is a follow-up of PR #400 about issue #393.
Most of the
mlfiles of the source folder have undergone anocp-indentcommand, followed by a quick review. Some remarks about this:core/lwt.mlandunix/lwt_unix.cppo.mluntouched, so as not to cause conflicts with ongoing PRs;ocp-indentingmlifiles wasn't really necessary, and it kept breaking comments indentation;ocp-indentgot the camlp4 and ppx files pretty wrong, and I indented them mostly by hand. Since they are quite obscure to me, these ones may require some extra attention.