-
Notifications
You must be signed in to change notification settings - Fork 175
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
[easy] Fix whitespace, formatting, prettify the code #393
Comments
One way to enforce sensible indentation throughout the whole codebase would be to add a I'll try to format some files with the default settings, just to see how it looks. |
We have the following
I am surprised that there isn't one in Lwt already. I don't know how much our configuration differs from the default, but its results are in line with the established OCaml style and with @aantron's example (I think). |
Yes, these are also my default settings (used when I don't have a If this is standard inside Ocsigen projects, I can go ahead and add it, then start "normalizing" existing files to match these settings. |
That sounds reasonable. How about converting one file first, say something large like Also, is it still easy to separate history into whitespace-only and logic-changing commits when using ocp-indent in a text editor? Or does it encourage conflating the two by making whitespace changes too eagerly? |
Scratch that, |
Well, this is entirely up to you, and the way you use your editor; ocp-indent is only here to help the editor auto-indenting the code. In Vim, for example, you can
I don't know much about Emacs and VSCode, but I'm sure they have similar features. |
Ok, that sounds good :) |
I've pushed a first commit where I've indented There is an option in ocp-indent to change that, just tell me what you think. Also, is it too early to open a PR, or should I open one and mark it as a WIP for the time being? |
It looks pretty good, actually. The only thing I'd say ocp-indent got wrong is the deprecation annotations ( For the match cases, I eventually agreed with indenting like ocp-indent does. I've taken to inserting blank lines when that makes the next match case difficult to find. We can insert them manually later (or now), in a hand-crafted prettification pass :) If you want to go through the whole file and make it beautiful, go ahead :) But I think this is already helpful, and worth a PR on its own. |
Thanks for the change. If willing to do more, I think basically the only files currently being worked on are |
About the C files, there is I'll give it a shot later today. |
Most of this has been done by @Richard-Degenne (thanks!). For ongoing formatting work, we just do it as we go, and, if needed, we can write a single sentence in |
When code doesn't look good, it is difficult to read and understand. We want Lwt to be easy to work on.
Basically, if you run into any strange indentation, please go ahead and fix it :) Here are some
immediateexamples from the recent past: [1] [2] [3] [4].It's recommended not to do this to a file that has an open, recent pull request, to avoid a merge conflict. "Recent" is probably less than a couple months old. Also, check any in-progress or easy issues for an idea of what is being worked on. On the other hand, don't worry too much – in case of a bad conflict, maintainers exist to make sure nobody has to do too much merging work :)
In particular, at the moment,
lwt.ml
(#354) probably shouldn't be touched, although you can edit the new one's whitespace by opening pull requests against thewat
branch that #354 comes from.Some pointers:
If you know how to use ocp-indent, teach us/me, or tell what it's capable of :)Thanks @Richard-Degenne, @vasilisp!This is an ongoing issue. Known to be in progress or done:
lwt_stream.ml
(Added .ocp-indent rules and fixed lwt_stream indentation #400, @Richard-Degenne).The text was updated successfully, but these errors were encountered: