-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add formatting script #102
base: master
Are you sure you want to change the base?
Conversation
5f1d228
to
6e71adc
Compare
Comments about the formatting:
Other thoughts? |
Meh, seems like a whole lot more trouble than it's worth... We do not too badly on the important stuff (consistency in naming things, where we put code, etc) and frankly I don't care about the whitespace stuff as long as it's not completely stupid (which this formatter seems to be). Let's revisit this once such tools have been deployed in large code bases (eg julia base) with nobody complaining too much. |
I do think having no space between operators is sometimes better for clarity. Eg |
Also no space in kwargs is sometimes nice. |
The point is that rules are context dependent. It might make sense to always have spaces around operators in non mathematically heavy code, but it doesn't when you're spending a lot of time writing polynomials. Whitespace can act to emphasize certain points, and the points you want to emphasize depend on the context. |
I don't fully agree. I agree it is a good idea to wait a bit longer until some bigger projects have used it, that I won't deny. I just wanted to give it a try and see what happens.
Modulo the polynomial aspect, where I agree, I think the consistency of always spaces is preferable. Probably subjective as always.
Yes, that's why you can disable it for sections. In my experience from the C++ world this happens pretty rarely.
Good point. Actually I would even go for the opposite (never spaces). Thanks for your comments. I'll certainly keep trying to convince you wrt. this, but for sure not top priority. |
d71f72f
to
62c6c37
Compare
Wow ... JuliaFormatter improved a lot. Especially it is much more configurable now. I know you don't like the idea @antoine-levitt, but I still propose that we use an automatic formatting tool as a default style. If for a particular context we want something else, we can just disable it for that area ... and they are working on heuristics to detect for example aligned equal signs and that stuff. Take a look at the code and tell me about the cases you dislike. I certainly have a few, but I think in general having this would be an improvement. For me it certainly found more cases where the code is now more readable rather than less. And where it is worse I think we just disable it. |
It doesn't actually improve code readability, because once you have a reasonably sane coding style (which we already do) anything extra is just personal preferences. Consistency is not a goal in itself, readability is. This kind of automated checks are good for very structured languages (C++, java) with large codebases and many decentralized collaborators. That's not really our case (yet?) Looking at the code the linebreaks in long function calls are excessive. The spacing of operators I maintain is better done manually (even if it's not consistent). Eg The changes I think are good are the automatic |
So you would be ok if this was only enforced on some of the things it is doing and plainly did not care about the others (e.g. operator spacing)? Because I think that could be done and would be a good start from my end. Another option I would be happy with is to not hard-enforce this in like Travis or so, but instead just use it as a base style. That would help me (because I can set vim to do the base formatting automatically) but one can still choose to commit something else. As for the polynomials that's definitely a case where I would disable formatting. That's clear, but I think there are not that may places where this is the case, actually, but we probably disagree on that, too ;). |
I do care about the spacing and the linebreaks, I think it makes the code actively worse. I don't understand why you insist so much on it, does it really bother you that much? I'm fine with the ; and the = if you want to merge that. |
Because automatic code formatting makes the life a lot easier. You just write your code, you don't have to care about aligning and indention, you save the file and it gives a nice standardised formatting. In case it looks awkward you most likely made a syntax error. I myself did not understand the fuss about it until I tried it a while back and noticed how much of all this became second nature. Like you find the bug just because you notice the place where the code starts to look odd. Especially that part I am missing a lot in DFTK, that's why I insist. Once I managed to set it up, just give it a serious try for a few weeks and maybe you'll see what I mean. |
So you want a code formatting pass to do the work of an IDE, basically? Can't you configure vim to do it? I just have emacs handle spacing and indentation (which has my custom preferred style, eg spacing eg for |
No I don't want it to do the work of an IDE, I want it to do the work instead of the IDE. Any IDE like vim / emacs / whatever will never be as good as parsing the Julia code as the Julia compiler will be, irrespective of how many years reimplementing the parser of the language in vimscript or emacs lisp you spend. One idea of the automated formatting tools like JuliaFormatter is to plainly leave that parsing to the compiler of the language itself and use the returned AST to style the code based on what it actually will be compiled to. Take the example of |
That's the point of LSP, for which there is a julia implementation. But parsing by hand source code has been what editors do for about fifty years, and it just works 99% of the time (and the rest you just do by hand)
Yeah in theory that's impossible. In practice you just grep for |
I disagree. It works maybe 90%, but in my experience automated formatting tools which use the language compiler (and the original formatting of the text) get it better ... whatever is left of that you can indeed do by hand. The reason why JuliaFormatter also looks at the original text is also the reason why just LSP does not cut it. Of course you could implement all formatting in an IDE which uses the LSP, that's true, but I am not aware of any vim plugin that does it. Also why solve the problem this way? You can just agree on a Julia tool to do that and write simple editor plugins for it, which only needs to pass and receive text. JuliaFormatter is already supported in plenty of editors. More that can use LSP for formatting, I'm pretty sure.
In this case you truly get 100%.
Sure, that's why JuliaFormatter is a standardised plugin for the IDEs just for formatting and not a Linter. |
The plan of this PR is to add a nice formatting script based on JuliaFormatter. I have not looked at the impact on the full code base, but generally I kind of like the result. Closes #88.
Edit I do not intend to commit the reformatted code for now, because that will lead to horrible conflicts in the other PRs. It's just for illustration purposes of the changes.