add format check action - initial repo format #2520

t-bltg · 2022-12-24T16:21:21Z

Description

add a format check action, running on PRs.
apply an initial format (options to be discussed :)).

I've ran on a single format error on https://github.com/MakieOrg/Makie.jl/blob/master/src/camera/camera.jl#L32-L34, which was trivial to fix, and I'm going to open an issue + PR in JuliaFormatter to fix this, but this is non-blocking.

Type of change

Delete options that do not apply:

Non functional change - format, code style

Checklist

Added an entry in NEWS.md (for new features and breaking changes)
Added or changed relevant sections in the documentation
Added unit tests for new algorithms, conversion methods, etc.
Added reference image tests for new plotting functions, recipes, visual options, etc.

CairoMakie/src/CairoMakie.jl

CairoMakie/src/display.jl

ffreyer · 2022-12-24T17:40:47Z

GLMakie/experiments/shaderabstr.jl

+    println(
+        io,
+        """
+in $(value_t) fragment_$(name)_index;
+uniform $(sampler_t) $(name)_texture;

-    vec4 get_$(name)(){
-        return texture($(name)_texture, fragment_$(name)_index);
-    }
-    """)
+vec4 get_$(name)(){
+    return texture($(name)_texture, fragment_$(name)_index);
+}
+""",
+    )


Would be good to ignore indention for this

This is a bit annoying yes (domluna/JuliaFormatter.jl#501), but still readable.

ffreyer · 2022-12-24T17:44:23Z

CairoMakie/src/overrides.jl

+function draw_poly(
+    scene::Scene,
+    screen::Screen,
+    poly,
+    points::Vector{<:Point2},
+    color::Union{Symbol, Colorant, Makie.AbstractPattern},
+    model,
+    strokecolor,
+    strokewidth,
+)


I prefer this to be double indented so that function .. end and ( .. ) are on different levels and visually distinct.

I think this is a matter of taste.

Sure, that's true for pretty much all of these. I'm mostly pointing this stuff out so it can be discussed and we can figure out what we actually want

src/basic_recipes/arrows.jl

src/interaction/inspector.jl

ffreyer · 2022-12-24T18:42:42Z

src/interaction/inspector.jl

+    z =
+        ((r - i) * img[l, b] + (i - l) * img[r, b]) * (t - j) +
+        ((r - i) * img[l, t] + (i - l) * img[r, t]) * (j - b)


I guess this is very subjective and nit-picky but I think in some situations (like here) it's nicer to have e.g. (t-j) instead of (t - j). The former sort of establishes groups and gives you less separate things to look at which, in my opinion, helps readability. Maybe this is something to keep unformated?

Not sure, I personally find this quite readable, and consistent with the rest.

ffreyer · 2022-12-24T18:45:38Z

src/interaction/iodevices.jl

@@ -4,206 +4,205 @@ represent keyboard buttons.
 """
 module Keyboard


I don't think we should lose the indention for modules with how we use them here.

src/makielayout/blocks/axis.jl

t-bltg · 2022-12-24T21:28:06Z

I have taken another approach, since there were too many controversial changes as @ffreyer pointed out.

I propose instead to use the minimal style, which e.g. doesn't take into account margins.

I'm also annoyed by the forced trailing zeros (20f0 -> 20.0f0) (see domluna/JuliaFormatter.jl#672), hopefully this will be resolved quickly. ==> EDIT: JuliaFormatter PR merged, I will update this PR asap.

ffreyer · 2022-12-24T22:51:56Z

I'll say it again here so it's more visible: Most of my complaints are just my opinions on code style. My main reason for posting them is so we can discuss those points and figure out what styling we want. It doesn't need to be what I suggest. But I think this pr is the place to discuss that.

t-bltg · 2022-12-24T23:18:43Z

Code style is always controversial, and comments on this PR are very welcome.
I'm glad we can have those discussions so that we can come up with an acceptable solution :)

t-bltg · 2023-03-02T12:44:19Z

Rebased against latest master.

Should this go in (based on #2493 (comment)) ?
This is exactly the purpose of this MR, not to waste time and endlessly discuss code style in other PRs.

SimonDanisch reviewed Dec 24, 2022

View reviewed changes

CairoMakie/src/CairoMakie.jl Outdated Show resolved Hide resolved

t-bltg force-pushed the fmt branch 3 times, most recently from fcf0d0d to ee0aa3c Compare December 24, 2022 16:45

t-bltg changed the title ~~Fmt~~ add formatter check action - initial repo format Dec 24, 2022

ffreyer reviewed Dec 24, 2022

View reviewed changes

CairoMakie/src/display.jl Show resolved Hide resolved

t-bltg force-pushed the fmt branch 2 times, most recently from 1e09a72 to 389be7a Compare December 24, 2022 17:39

ffreyer reviewed Dec 24, 2022

View reviewed changes

t-bltg force-pushed the fmt branch 2 times, most recently from 440bcff to 2a15ad7 Compare December 24, 2022 21:25

t-bltg mentioned this pull request Dec 24, 2022

add trailing_zero option domluna/JuliaFormatter.jl#672

Merged

t-bltg force-pushed the fmt branch from 2a15ad7 to ce4f966 Compare December 24, 2022 23:17

t-bltg force-pushed the fmt branch from ce4f966 to 6be9044 Compare December 24, 2022 23:24

t-bltg changed the title ~~add formatter check action - initial repo format~~ add format check action - initial repo format Dec 28, 2022

t-bltg force-pushed the fmt branch 4 times, most recently from 20aa8db to 3e58ffe Compare December 31, 2022 17:13

t-bltg force-pushed the fmt branch 3 times, most recently from 1397143 to 781c1f6 Compare January 9, 2023 19:28

t-bltg force-pushed the fmt branch from 781c1f6 to fa47cc6 Compare January 19, 2023 10:50

t-bltg force-pushed the fmt branch 4 times, most recently from e55ee76 to eeeea7c Compare March 2, 2023 11:57

t-bltg added 11 commits March 2, 2023 12:59

add format check action

01456cd

edit .JuliaFormatter.toml

b995e4c

add format note in CONTRIBUTING.md

9153a56

restrict directories

b18a5f7

fix formatter ambiguity

eaabbfe

reduce options

d5e6976

run the check in a temp env

361179e

change whitespace

a7cb969

use minimal style

d95c4e2

rework note

5d55b55

use ignore in .toml

296fd9d

t-bltg force-pushed the fmt branch from eeeea7c to 8e7ca6e Compare March 2, 2023 11:59

rework CI formatter check

74224b0

t-bltg force-pushed the fmt branch from 8e7ca6e to 4ee1219 Compare March 2, 2023 12:03

ffreyer mentioned this pull request Apr 13, 2024

Inconsistent capitalisation of plot recipes names #3784

Open

t-bltg mentioned this pull request May 5, 2024

document WilkinsonTicks #3819

Merged

7 tasks

Merge branch 'master' into fmt

db9c4e8

t-bltg force-pushed the fmt branch from 4ee1219 to c3adb3d Compare May 5, 2024 10:25

update CHANGELOG

daf7201

t-bltg force-pushed the fmt branch from c3adb3d to 88d9505 Compare May 5, 2024 10:27

t-bltg added 2 commits May 5, 2024 12:34

update action

a73d1c7

initial format

1215ffa

t-bltg force-pushed the fmt branch from 88d9505 to 1215ffa Compare May 5, 2024 10:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add format check action - initial repo format #2520

add format check action - initial repo format #2520

t-bltg commented Dec 24, 2022 •

edited

Loading

ffreyer Dec 24, 2022

t-bltg Dec 24, 2022 •

edited

Loading

ffreyer Dec 24, 2022

t-bltg Dec 24, 2022

ffreyer Dec 24, 2022

ffreyer Dec 24, 2022

t-bltg Dec 24, 2022

ffreyer Dec 24, 2022

t-bltg commented Dec 24, 2022 •

edited

Loading

ffreyer commented Dec 24, 2022

t-bltg commented Dec 24, 2022 •

edited

Loading

t-bltg commented Mar 2, 2023

		@@ -4,206 +4,205 @@ represent keyboard buttons.
		"""
		module Keyboard

add format check action - initial repo format #2520

Are you sure you want to change the base?

add format check action - initial repo format #2520

Conversation

t-bltg commented Dec 24, 2022 • edited Loading

Description

Type of change

Checklist

ffreyer Dec 24, 2022

Choose a reason for hiding this comment

t-bltg Dec 24, 2022 • edited Loading

Choose a reason for hiding this comment

ffreyer Dec 24, 2022

Choose a reason for hiding this comment

t-bltg Dec 24, 2022

Choose a reason for hiding this comment

ffreyer Dec 24, 2022

Choose a reason for hiding this comment

ffreyer Dec 24, 2022

Choose a reason for hiding this comment

t-bltg Dec 24, 2022

Choose a reason for hiding this comment

ffreyer Dec 24, 2022

Choose a reason for hiding this comment

t-bltg commented Dec 24, 2022 • edited Loading

ffreyer commented Dec 24, 2022

t-bltg commented Dec 24, 2022 • edited Loading

t-bltg commented Mar 2, 2023

t-bltg commented Dec 24, 2022 •

edited

Loading

t-bltg Dec 24, 2022 •

edited

Loading

t-bltg commented Dec 24, 2022 •

edited

Loading

t-bltg commented Dec 24, 2022 •

edited

Loading