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

github/workflows: add editorconfig linting #15135

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

Akemi
Copy link
Member

@Akemi Akemi commented Oct 20, 2024

alternative or additional to #15132
follow up of #15124

IndentSize disabled because it checks all indentation for multiple of indent_size, so spaces for alignment error too.
MaxLineLength disabled because we don't have a hard limit on 80, many/some files also go over 100, disabled in other linters too.
excluding osdep/dirent-win.h since it's a straight copy and we decided to keep it as is.

this checks all files like defined in our .editorconfig and not just c-family files like clang-format(?).

in the upcoming release (3.1) they deprecated the .ecrc file in favour of the newly added .editorconfig-checker.json. so i decided to use the new one instead (it's not auto discovered yet with the current version). the upcoming release will also add github-actions formatting, so errors will also be displayed in the code/diff view of the PR similar to the swift linting. added this to the config file already, it is ignored with the current version.

will fix the two remaining errors either here, if we decide to go with this, or in a separate PR.

@kasper93 i hope you don't mind this.

Copy link

github-actions bot commented Oct 20, 2024

Download the artifacts for this pull request:

Windows
macOS

.editorconfig-checker.json Outdated Show resolved Hide resolved
.editorconfig-checker.json Show resolved Hide resolved
.github/workflows/lint.yml Outdated Show resolved Hide resolved
@Akemi Akemi force-pushed the lint_editorconfig branch 3 times, most recently from cb1b453 to 095976f Compare October 21, 2024 20:19
@Akemi
Copy link
Member Author

Akemi commented Oct 21, 2024

seems like the go install command doesn't install the binary globally. probably because the GOBIN env var is not set? trying a few things now.

@Akemi Akemi force-pushed the lint_editorconfig branch 3 times, most recently from 84403e8 to 7da2814 Compare October 21, 2024 21:00
@Akemi
Copy link
Member Author

Akemi commented Oct 21, 2024

instead of the setup-go action we could also just call ~/go/bin/editorconfig-checker. go install seems to install it to the home directory (~/go/bin/) by default/in our case.

had to exclude the TOOLS/osxbundle/mpv.app/Contents/PkgInfo too, since it seems to be expected to have no new newline at the end of the file.

@kasper93
Copy link
Contributor

kasper93 commented Oct 21, 2024

instead of the setup-go action we could also just call ~/go/bin/editorconfig-checker. go install seems to install it to the home directory (~/go/bin/) by default/in our case.

Is this maybe equivalent to $GOBIN/editorconfig-checker?

EDIT: If install works without setup-go, it should already have GOBIN defined IIRC. So we can try to call directly.

@Akemi
Copy link
Member Author

Akemi commented Oct 21, 2024

sadly $GOBIN/editorconfig-checker doesn't work either https://github.com/mpv-player/mpv/actions/runs/11449449084/job/31855050027?pr=15135

@Akemi
Copy link
Member Author

Akemi commented Oct 21, 2024

alternative GOBIN=/usr/local/bin/ go install?

[edit]
or would you prefer me using this action instead. it's the official one of the editorconfig-checker org, which makes me much more comfortable. it's a lot more widely used. tbh i meant to use this one but had the other one still open in another tab and ended up copying the wrong one. sorry for messing that one up.

@kasper93
Copy link
Contributor

or would you prefer me using this action instead. it's the official one of the editorconfig-checker org

looks good, we can use that.

@kasper93 kasper93 merged commit 96095bd into mpv-player:master Oct 24, 2024
25 checks passed
@Akemi Akemi deleted the lint_editorconfig branch October 24, 2024 16:42
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.

2 participants