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

Add formatting script #102

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Add formatting script #102

wants to merge 2 commits into from

Conversation

mfherbst
Copy link
Member

@mfherbst mfherbst commented Jan 15, 2020

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.

@mfherbst
Copy link
Member Author

Comments about the formatting:

  • Sometimes data is wrapped in a rather lengthy manner. That's not too big a problem as one could manually disable the autoformat for those regions (means an extra comment of no information, however)
  • Function definitions and calls with one argument per line are pretty long (e.g. check the magnesium PBE example). Disabling that each time explicitly is unrealistic. That's a real deal breaker for me. (Issue: How to disable formatting of function calls? domluna/JuliaFormatter.jl#144).

Other thoughts?

@antoine-levitt
Copy link
Member

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.

@antoine-levitt
Copy link
Member

antoine-levitt commented Jan 16, 2020

I do think having no space between operators is sometimes better for clarity. Eg f(a*b, c) is sometimes more readable than f(a * b, c), and esp in more complex operations things like a*b + c are nice. Also manual alignment in some arrays and is completely destroyed by this script.

@antoine-levitt
Copy link
Member

Also no space in kwargs is sometimes nice.

@antoine-levitt
Copy link
Member

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.

@mfherbst
Copy link
Member Author

Meh, seems like a whole lot more trouble than it's worth...

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.

Eg f(a*b, c) is sometimes more readable than f(a * b, c)

Modulo the polynomial aspect, where I agree, I think the consistency of always spaces is preferable. Probably subjective as always.

Also manual alignment in some arrays and is completely destroyed by this script.

Yes, that's why you can disable it for sections. In my experience from the C++ world this happens pretty rarely.

Also no space in kwargs is sometimes nice.

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.

@mfherbst
Copy link
Member Author

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.

@antoine-levitt
Copy link
Member

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 a*x + b, 1/2 * x^2, etc. It messes up manual alignment, eg in spherical_harmonics.jl. It's also inconsistent in how it treats .+ vs +. Typing of struct fields as Any is unnecessarily verbose.

The changes I think are good are the automatic ; instead of , before kwargs, and some of the renaming for x = 1:n instead of for x in 1:n.

@mfherbst
Copy link
Member Author

mfherbst commented Sep 28, 2020

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 ;).

@antoine-levitt
Copy link
Member

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)?

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.

@mfherbst
Copy link
Member Author

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.

@antoine-levitt
Copy link
Member

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 = but not for +), and then overwrite when needed.

@mfherbst
Copy link
Member Author

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 for in versus for =. In JuliaFormatter it actually determines the types of the things in the expression and uses that to decide between the two. Try doing that natively in the IDE ...

@antoine-levitt
Copy link
Member

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

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)

In JuliaFormatter it actually determines the types of the things in the expression and uses that to decide between the two. Try doing that natively in the IDE ...

Yeah in theory that's impossible. In practice you just grep for : and you get 99% of use-cases. But again that's the task of an IDE, not of a linter.

@mfherbst
Copy link
Member Author

mfherbst commented Sep 28, 2020

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)

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.

Yeah in theory that's impossible. In practice you just grep for : and you get 99% of use-cases.

In this case you truly get 100%.

But again that's the task of an IDE, not of a linter.

Sure, that's why JuliaFormatter is a standardised plugin for the IDEs just for formatting and not a Linter.

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.

Automatic source-code formatting
2 participants