open-AIMS / ADRIA.jl

ADRIA: Adaptive Dynamic Reef Intervention Algorithms. A multi-criteria decision support platform for informing reef restoration and adaptation interventions.
MIT License
14 stars 5 forks source link

Julia format GH workflow, format all files #790

Closed arlowhite closed 1 month ago

arlowhite commented 3 months ago

Adds julia-format GitHub workflow action. Add .JuliaFormatter.toml Update development_setup.md format all files.

Also see Notion notes

Developer Setup Change

Recommendation is to uninstall the Julia Formatter plugin (or disable for your ADRIA.jl workspace if you use it in other projects). The primary VSCode Julia plugin uses .JuliaFormatter.toml, making the plugin redundant; the plugin only uses its own config and ignores .JuliaFormatter.toml. So having both installed/active adds confusion with around which formatting config is being applied.

Formatter Config Changes

Changes from what we were using before with the formatter plugin. These settings reduce formatter changes:

Note: always_for_in config is strange. false seems to be always =, true is in (or value of for_in_replacement). nothing makes no change.

ConnectedSystems commented 3 months ago

seems like there's a rule that a semicolon always goes before named args. in some cases this seems awkward to me.

Yes, this is Julia syntax. Keyword arguments are specified after a semi-colon.

arlowhite commented 3 months ago

I was searching to see if there's a way to preserve git blame history. The best way seems to be --ignore-revs-file https://medium.com/codex/how-to-introduce-a-code-formatter-without-messing-up-git-history-4a16bd074c10 So IDE git blame features would ignore this commit.

EDIT: better to use native git support for this https://github.com/gitkraken/vscode-gitlens/issues/947#issuecomment-583703079 could commit the file, but everyone would need to run git config --local blame.ignoreRevsFile .git-blame-ignore-revs

Zapiano commented 3 months ago

Since we have a very critical/delicate PR, almost ready to merge, changing more than 20 files, I think say we should wait for that one to be merged before this.

PS: the PR in question is #760

ConnectedSystems commented 2 months ago

@arlowhite @Zapiano is the blocker for this PR now resolved?

Zapiano commented 2 months ago

@arlowhite @Zapiano is the blocker for this PR now resolved?

My understanding we decided to wait until #760 is merged to main.

arlowhite commented 1 month ago

In our last meeting, we discussed getting this in sooner than later and allowing PRs to merge with this check failing if necessary. I've rebased and reformatted.

arlowhite commented 1 month ago

@Zapiano do you still want me to leave src/scenario.jl untouched? or are you ok with these formatting changes? https://github.com/open-AIMS/ADRIA.jl/pull/790/files#diff-bd5b8c618631550cda060d53f80beee397a2975f98de66d5a123ed45199ed86c

Zapiano commented 1 month ago

@Zapiano do you still want me to leave src/scenario.jl untouched? or are you ok with these formatting changes? https://github.com/open-AIMS/ADRIA.jl/pull/790/files#diff-bd5b8c618631550cda060d53f80beee397a2975f98de66d5a123ed45199ed86c

I am going to merge #817 today, after that you can format the entire project and I review it. Is that ok?

arlowhite commented 1 month ago

What's your preference here? returning assignment expressions seems a bit weird to me: return data .= (data .- mi) ./ (ma - mi) so, I'm thinking this is better:

data .= (data .- mi) ./ (ma - mi)
return data
ConnectedSystems commented 1 month ago

Agree with your thinking.

@arlowhite I'll have a closer look at one of the larger files when I have the time today (likely late afternoon unfortunately).

ConnectedSystems commented 1 month ago

Not a fan of these forced replacement of in to but I guess it's okay...

for r ∈ 1:n_rows
        for c ∈ 1:n_cols
Zapiano commented 1 month ago

@arlowhite Now you can rebase and format all files. Thanks for waiting (and sorry it took so long).

arlowhite commented 1 month ago

I've disabled the for in change and function expansion. The documented defaults are false, but I think blue style turns them on.

arlowhite commented 1 month ago

Not a fan of these forced replacement of in to but I guess it's okay...

for r ∈ 1:n_rows
        for c ∈ 1:n_cols

I've disabled it.

arlowhite commented 1 month ago

Only one place in code doesn't use for in src\io\rme_result_io.jl location_centroids = [centroid(multipoly) for multipoly ∈ geodata.geom]

So we might want to do

always_for_in=true
for_in_replacement="in"

Unless some developers want to use the math symbol in some cases.

EDIT: Takuya on Teams: "Sometimes it's nice to use ∈, other times "in" is fine" so leaving config as is, which will not change for in symbols.