Introduce JuliaFormatter with style=blue #220
Conversation
|
I like this a lot 👍 |
| @@ -113,10 +119,10 @@ include("zygote_adjoints.jl") | |||
| include("test_utils.jl") | |||
|
|
|||
| function __init__() | |||
| @require Kronecker="2c470bb0-bcc8-11e8-3dad-c9649493f05e" begin | |||
| @require Kronecker = "2c470bb0-bcc8-11e8-3dad-c9649493f05e" begin | |||
There was a problem hiding this comment.
It seems a bit inconsistent to add whitespace here, compared with the conventions for keyword arguments - is this according to BlueStyle or a JuliaFormatter thing?
There was a problem hiding this comment.
I agree. I suspect this is a JuliaFormatter (or CSTParser) issue
|
Did the merge commit revert any changes from JuliaFormatter? I guess it would be good to add the Github action for checking the format in this PR as well to be sure it passes. |
.github/workflows/ci.yml
Outdated
| @@ -47,3 +47,10 @@ jobs: | |||
| with: | |||
| github-token: ${{ secrets.GITHUB_TOKEN }} | |||
| path-to-lcov: ./lcov.info | |||
| format: | |||
There was a problem hiding this comment.
Separate from the others as format-testing should be independent of the host/version matrix and it seems more sensible to only run it once (and then it's easier to distinguish format failure from the "regular" tests)
.github/workflows/ci.yml
Outdated
| # The version of JuliaFormatter used is just to show how to specify the version. The latest | ||
| # version would be preferrable. | ||
| run: | | ||
| julia -e 'using Pkg; Pkg.add(PackageSpec(name="JuliaFormatter", version="0.12.2"))' |
There was a problem hiding this comment.
I don't like that the version number has to be updated manually if there is a new version of JuliaFortmatter. We could just install the latest version but this means we would end up with breaking changes without realizing. Ideally, we would add a Project.toml file in some directory and be notified about breaking releases by CompatHelper.
There was a problem hiding this comment.
I agree, that'd be nice. I'm not sure how you'd go about sorting it out with the Project.toml (feel free to make those changes on this branch or a separate PR). I do think it's overall better to pin the version of the formatter, as otherwise it can be hard to figure out how exactly to get the local formatter be in sync with the cloud format-checker..
There was a problem hiding this comment.
I'm not sure how you'd go about sorting it out with the Project.toml
Similar to the package, tests, and documentation, we can specify the SemVer-compatible versions in a separate project environment. CompatHelper will automatically create a PR on Github if a new breaking release is available (we are not notified about non-breaking releases).
it can be hard to figure out how exactly to get the local formatter be in sync with the cloud format-checker.
I think specifying the version that we use in a project environment is the best we can do. We can't control which version of JuliaFormatter developers use in their editors or install globally but if they load our project environment it will always be compatible.
Co-authored-by: David Widmann <[email protected]>
Co-authored-by: David Widmann <[email protected]>
|
I simplified the Github action and pushed some changes, but since I got involved now someone else has to approve the PR. |
|
@devmotion I think you still need to indicate that your change requests have been satisfied. |
Introduces a format check (using JuliaFormatter / https://github.com/julia-actions/julia-format) with
style = "blue". Resolves #219.