Agents.jl icon indicating copy to clipboard operation
Agents.jl copied to clipboard

Add .JuliaFormatter.toml for contributers

Open jacobusmmsmit opened this issue 2 years ago • 39 comments

This pull request adds a configuration file to be used with JuliaFormatter.jl, the built-in formatter of Julia for VSCode and the most popular one in general.

This will allow a consistent style to be enforced throughout the repository and make the code more similar to related packages in the Julia ecosystem.

jacobusmmsmit avatar Dec 24 '22 22:12 jacobusmmsmit

oof I don't like this formatting style at all... I like only 1 normal additional identitation level when having open functions such as

name(
    arg1, arg2;
    kwarg1, kwarg2
)

I guess I would need to study the julia formatter docs to see how to achieve this.

Datseris avatar Dec 24 '22 22:12 Datseris

Also, let's put the format check CI here as well, as shown here: https://github.com/SciML/SciMLStyle#juliaformatter

Datseris avatar Dec 24 '22 22:12 Datseris

I added the file to CI and changed its triggers to be the same as the current ci.yml. I wasn't sure where to fit it into that file, or whether it even belonged in there, so I made a new one.

jacobusmmsmit avatar Dec 24 '22 23:12 jacobusmmsmit

I've read the Jula formatter but I don't know which option corresponds to this: https://github.com/JuliaDynamics/Agents.jl/pull/723#issuecomment-1364590737 :(

if you know please let me know?

Datseris avatar Dec 25 '22 08:12 Datseris

Honestly, I don't know myself. Might be best to ask the JuliaFormatter team themselves

jacobusmmsmit avatar Dec 25 '22 13:12 jacobusmmsmit

Ok so Blue style is closer to what you prefer, from what I can tell. It's also my preferred one over SciML by far. What do you think?

jacobusmmsmit avatar Mar 08 '23 18:03 jacobusmmsmit

Btw, in the case of hard formatting, maybe it's best to just enforce it in src, often in examples and docs it's nice to be able to go against strict guidelines, but when 10 people are contributing to a project automatically enforcing a style is nice.

jacobusmmsmit avatar Mar 08 '23 18:03 jacobusmmsmit

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (37d3a13) 92.27% compared to head (a4b302c) 88.58%.

:exclamation: Current head a4b302c differs from pull request most recent head 3f5d6d5. Consider uploading reports for the commit 3f5d6d5 to get more accurate results

Files Patch % Lines
src/spaces/graph.jl 40.00% 9 Missing :warning:
src/Agents.jl 66.66% 3 Missing :warning:
src/deprecations.jl 0.00% 3 Missing :warning:
src/spaces/grid_multi.jl 89.28% 3 Missing :warning:
src/spaces/walk.jl 86.66% 2 Missing :warning:
src/core/model_abstract.jl 75.00% 1 Missing :warning:
src/simulations/sample.jl 66.66% 1 Missing :warning:
src/submodules/pathfinding/astar.jl 80.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #723      +/-   ##
==========================================
- Coverage   92.27%   88.58%   -3.69%     
==========================================
  Files          33       30       -3     
  Lines        2277     1963     -314     
==========================================
- Hits         2101     1739     -362     
- Misses        176      224      +48     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 08 '23 18:03 codecov-commenter

oh yeah we really have to do finish this but I haven't had the time yet. I don't like current style at all though. I don't like that it enforces a line break after every single argument, I find it super pointless...

Datseris avatar Mar 09 '23 08:03 Datseris

Ok, so this new style I made by altering MinimalStyle is pretty... well, minimal in the changes that it makes. The biggest thing is that because margin = 10_000, there will never be any changes to how arguments are handled. I'm sure there's a more elegant solution to this, but it does, at least, allow us to enforce things like:

  • Always explicitly returning with return,
  • Consistent use of whitespace,
  • Trailing commas in iterables,
  • Removing unnecessary newlines,
  • Adding newlines to the end of files,

Personally, I'd prefer if whitespace_in_typedefs were false as our type definitions are quite long and benefit from being compacted. My most recent commit does this.

Finally, before this is merged (if it is), there might be a better way to describe the formatting than what we have i.e. if we can specify style = "minimal" with only the changes we want to this, rather than describing "minimal" and our additional changes.

jacobusmmsmit avatar Mar 09 '23 12:03 jacobusmmsmit

Yeah this is where we have a problem:

image

that's the one thing that really bothers me that the function has a closing parenthesis at 0 identation. There is no way/option you are aware of that makes it so that the function arguments and closing parenthesis are idented one more time?

Datseris avatar Mar 09 '23 16:03 Datseris

? Finally, before this is merged (if it is), there might be a better way to describe the formatting than what we have i.e. if we can specify style = "minimal" with only the changes we want to this, rather than describing "minimal" and our additional changes.

This will become a style for JuliaDyunamics which I'll apply to all repos. So I'll do a PR to the Julia formatter package to add this style.

Datseris avatar Mar 09 '23 16:03 Datseris

There is no way/option you are aware of that makes it so that the function arguments and closing parenthesis are idented one more time?

I'm not 100% sure if this is what you mean but check out YAS style guide which is inspired by the one used in JuMP. Specifically, see here, is this what you're after?

After looking at my preferred compared to YAS style, they are actually very close: YAS is slightly more strict as it additionally imposes

  1. A margin limit (which makes the biggest difference)
  2. Replace x |> f with f(x) (fair enough, we're not doing any data analysis here so pipes are rare)
  3. Replace import with using (only when interchangeable afaik)
  4. Annotate untyped struct fields with Any (also fair enough, we shouldn't have untyped fields in structs anyway)

If you want to try out YAS, cd into Agents.jl on main or some branch and run

using JuliaFormatter; format("src/.", YASStyle())

jacobusmmsmit avatar Mar 09 '23 16:03 jacobusmmsmit

Grrrrr this is so annoying. here is what I want:

funccall(
    arg1,
    arg2,
    arg3,
)
    x = 2 # operations
end

to instead be

funccall(
        a, b, c, # same row unless they exceed margin
        d, e, g, # notice that this is twice idented, one more from original
    )
    x = 2 # operations
end

but I have no idea how to achieve this based on the current documentation of the formatter. These are the only changes that I care about implementing. For everything else I'm happy to follow the blue style.

Datseris avatar Mar 09 '23 18:03 Datseris

Wait, this is exactly what SciML recommends... I wonder what doesn't work here.

jacobusmmsmit avatar Mar 10 '23 13:03 jacobusmmsmit

Yes but SciML indents on the function opening parenthesis, which is bad if functions have long names. I want to ident one level more than the funtion's natural identation :D

Maybe I am asking for too much and we should just go with blue style... :(

Datseris avatar Mar 10 '23 14:03 Datseris

Wait but Blue says to do exactly that (scroll down a bit), it just treats kwargs differently. Maybe Blue is the one true style?

jacobusmmsmit avatar Mar 10 '23 14:03 jacobusmmsmit

I'm not sure blue style is correct. Here is what I want:

function f(
        a, b, c, # same row unless they exceed margin
        d, e, g, # notice that this is twice idented, one more from original
    )
    x = 2 # operations
end

here is what Blue style does, at least when used with JuliaFormatter.jl:

function f(
    a,
    b,
    c,
    d,
    e,
    f,
)
    x = 2 # operations
end

so there are two differnces: first that I don't want a line break after every single argument, only when they exceed the 92 chars, and second that I prefer a second level of identation when writing the function arguments, so that the function closing parenthesis has 1 level of identation, just like the function code.

I can live with second thing being unsatisfied. I can live with this:

function f(
    a, b, c, # same row until 92 chars
    e, f, g,
)
    x = 2
end

but what I cannot live with, 100%, is enforcing a line break after every single function argument.

Datseris avatar Mar 10 '23 14:03 Datseris

with everything else in the blue style I am completely fine with btw. Let's just try to see if we can address this issue and if not, we give up and go with blue style... :(

Datseris avatar Mar 10 '23 14:03 Datseris

I like not to have line breaks after each argument too, fortunately if I read correctly actually the SciML format should be okay?

From the docs:

If the number of arguments is too large to fit into a 92 character line, then use as many arguments as possible within a line and start each new row with the same indentation, preferably at the same column as the ( but this can be moved left if the function name is very long.

Tortar avatar Oct 24 '23 00:10 Tortar

The example given to this point:

# Yes
function my_large_function(argument1, argument2,
                           argument3, argument4,
                           argument5, x, y, z)

# No
function my_large_function(argument1,
                           argument2,
                           argument3,
                           argument4,
                           argument5,
                           x,
                           y,
                           z)


This is exactly what @Datseris wrote in his example above.

jacobusmmsmit avatar Oct 24 '23 10:10 jacobusmmsmit

yes the only difference is that this:

function my_large_function(
    argument1, argument2, argument3, 
    argument4, argument5, x, y, z)
    s = 3
end

in SciMLStyle will be converted to

function my_large_function(argument1, argument2, argument3,
    argument4, argument5, x, y, z)
    s = 3
end

but even these sintaxes are allowed:

function my_large_function(argument1, 
    argument2, argument3, argument4, 
    argument5, x, y, z)
    s = 3
end

function my_large_function(argument1, 
    argument2, argument3, 
    argument4, argument5, 
    x, y, z)
    s = 3
end

the difference is then that it wants to put at least one argument at the name of the function level. it doesn't sound bad to me.

But It isn't very enforcing since one can choose one or the other way and it will be allowed. Maybe this is a weak point though

Tortar avatar Oct 24 '23 11:10 Tortar

Last option: make a new function definition setting an option :D

Maybe it is not that hard to create a new setting like this in JuliaFormatter, who knows

Tortar avatar Oct 24 '23 14:10 Tortar

I think this is the option to consider. If not possible, we just go with:

function my_large_function(argument1, argument2, argument3,
    argument4, argument5, x, y, z)
    s = 3
end

which is what the SCimlStyle will do (I think).

p.s. notice that this PR needs to be re0opened due to the huge amount of code changes that have happened in the meantime.

Datseris avatar Oct 24 '23 14:10 Datseris

Actually 3 weeks ago the SciML style changed enforcing the double-indent style so now it is usable as it is! See https://github.com/domluna/JuliaFormatter.jl/pull/776

Tortar avatar Nov 19 '23 18:11 Tortar

p.s. notice that this PR needs to be re0opened due to the huge amount of code changes that have happened in the meantime.

I think we should keep it on the same PR for discoverability, but yes throw away the changes, pull from main, and add a formatting merge commit.

jacobusmmsmit avatar Nov 19 '23 18:11 jacobusmmsmit

I think we should keep it on the same PR for discoverability, but yes throw away the changes, pull from main, and add a formatting merge commit.

ok, can you do that with the new SciML formatter?

Datseris avatar Nov 19 '23 22:11 Datseris

Strangely, when I run JuliaFormatter.format() I get a parsing error in src/core/model_standard.jl. I'm using the latest version of JuliaFormatter 1.0.42, on Julia 1.9.4. The parsing error is reported to be on the last line.

Can anyone else try checking out main, adding the newest version of JuliaFormatter to their base env and running using JuliaFormatter; JuliaFormatter.format("src")?

Additionally precompiling I get the following error:

(Agents) pkg> precompile
Precompiling project...
  ✗ Agents
  0 dependencies successfully precompiled in 6 seconds. 104 already precompiled.

ERROR: The following 1 direct dependency failed to precompile:

Agents [46ada45e-f475-11e8-01d0-f70cc89e6671]

Failed to precompile Agents [46ada45e-f475-11e8-01d0-f70cc89e6671] to "/Users/smit/.julia/compiled/v1.9/Agents/jl_d8Uy3I".
ERROR: LoadError: MethodError: no method matching namify(::Nothing)

jacobusmmsmit avatar Nov 20 '23 15:11 jacobusmmsmit

Already tried...same error as you, it is probably a bug in the formatter...

Also, let's put the format check CI here as well, as shown here: https://github.com/SciML/SciMLStyle#juliaformatter

this makes me think that enforcing it on CI could be just too much struggle for contributors.

If you try JuliaFormatter.format(".") you'll find even more errors :D

Tortar avatar Nov 20 '23 16:11 Tortar

Judging from ERROR: LoadError: MethodError: no method matching namify(::Nothing) it probably messed up the @agent macro

Tortar avatar Nov 20 '23 16:11 Tortar