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

Add editorconfig and update bash files accordingly #5446

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

b10n1k
Copy link
Contributor

@b10n1k b10n1k commented Jan 30, 2024

https://progress.opensuse.org/issues/153793
Pull latest changes of .editorconfig from os-autoinst-common, shfmt to the
dependencies and enable CI checks.

recreate #5440

subrepo:
  subdir:   "external/os-autoinst-common"
  merged:   "fbd66e7f6"
upstream:
  origin:   "git@github.com:os-autoinst/os-autoinst-common.git"
  branch:   "master"
  commit:   "fbd66e7f6"
git-subrepo:
  version:  "0.4.6"
  origin:   "???"
  commit:   "???"
@okurz
Copy link
Member

okurz commented Jan 30, 2024

@Martchus why are javascript issues still showing up like that?

And
https://app.circleci.com/pipelines/github/os-autoinst/openQA/12852/workflows/e01e4366-9228-4b2b-829a-825f2c36f18a/jobs/119783

says
"dependencies.yaml
189:3 error duplication of key "shfmt" in mapping (key-duplicates)"

@b10n1k b10n1k force-pushed the update_ext_after_master_rebase branch 2 times, most recently from d06d152 to db66633 Compare January 30, 2024 08:15
Comment on lines +328 to 331
shfmt-3.5.1
setxkbmap-1.3.1
ShellCheck-0.8.0
shfmt-3.5.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
shfmt-3.5.1
setxkbmap-1.3.1
ShellCheck-0.8.0
shfmt-3.5.1
setxkbmap-1.3.1
ShellCheck-0.8.0
shfmt-3.5.1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have been fixed

@b10n1k
Copy link
Contributor Author

b10n1k commented Jan 30, 2024

eslint starts failing once the .editorconfig is created.
The "prettier/prettier": "error" in .eslintrc.json is causing the errors

@okurz
Copy link
Member

okurz commented Jan 30, 2024

eslint starts failing once the .editorconfig is created. The "prettier/prettier": "error" in .eslintrc.json is causing the errors

Ok, so as we don't have plans to merge PRs with failing checks what are your intentions about it?

@@ -0,0 +1,12 @@
[*]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just stumbled upon this issue: mvdan/sh#664
And it mentions [[shell]]. I can't find that in the shfmt documentation nor in the EditorConfig documentation but it seems to work :)

The problem seems to be that eslint is using the editorconfig as well, and as we are using * we can't blame it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nevermind, I misread the diff.
It was apparently touching the right files when i used [[shell]], but then it was using the default formatting rules, using tabs etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried [script/**] instead, but it still touched files elsewhere (and did not touch anything under script). Weird. Have to work on other tasks now though...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently this works, but we can't use the config file then:

shfmt -w -i 4 -bn -ci -sr $(shfmt -f .)

e.g.

shfmt -w $(shfmt -f .)

does not use .editorconfig

So -f is able to find the right files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the other way around?

[*.js]
ignore = true

Makefile Outdated
.PHONY: test-shfmt
test-shfmt:
@which shfmt >/dev/null 2>&1 || (echo "Command 'shfmt' not found, can not execute bash script syntax checks" && false)
shfmt -d .
Copy link
Contributor

@perlpunk perlpunk Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to do the following right now:

Suggested change
shfmt -d .
shfmt -d -i 4 -bn -ci -sr $(shfmt -f .)

and remove the .editorconfig symlink.
We can think of any improvements later and get this big PR merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering we were not able to make eslint/prettier ignore the editor config (after reading https://github.com/prettier/eslint-plugin-prettier, https://prettier.io/docs/en/configuration.html, https://prettier.io/docs/en/options.html and https://eslint.org/docs/latest/use/configure/configuration-files#using-configuration-files and trying several variants) that might be the best way to move this forward for now, indeed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And apparently

shfmt -d -i 4 -bn -ci -sr .

also works. Not sure why we ruled that out when we experimented with it in the scripts repo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right, because of the .bpan directory which we wanted to ignore.
So we might need something like this, depending on the repo:

shfmt -d -i 4 -bn -ci -sr  $(shfmt -f . | grep -v '^.bpan' )

@b10n1k b10n1k force-pushed the update_ext_after_master_rebase branch 2 times, most recently from dbea6d6 to f5f2021 Compare January 30, 2024 12:51
Makefile Outdated
.PHONY: test-shfmt
test-shfmt:
@which shfmt >/dev/null 2>&1 || (echo "Command 'shfmt' not found, can not execute bash script syntax checks" && false)
shfmt -d -d -i 4 -bn -ci -sr $(shfmt -f . | grep -v '^.bpan' )
Copy link
Contributor

@perlpunk perlpunk Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't work as it is a Makefile, where shell syntax doesn't work.
Also we don't have .bpan here in this repo, so maybe don't use it.
Also the -d option is duplicate.
Try this:

Suggested change
shfmt -d -d -i 4 -bn -ci -sr $(shfmt -f . | grep -v '^.bpan' )
shfmt -d -i 4 -bn -ci -sr $$(shfmt -f .)

With the double dollar it should work.

@b10n1k b10n1k force-pushed the update_ext_after_master_rebase branch 4 times, most recently from e9f6c76 to b261327 Compare January 30, 2024 13:28
https://progress.opensuse.org/issues/153793
Pull latest changes of .editorconfig from os-autoinst-common, shfmt to the
dependencies and enable CI checks.

Signed-off-by: ybonatakis <ybonatakis@suse.com>
@b10n1k b10n1k force-pushed the update_ext_after_master_rebase branch from b261327 to b17990c Compare January 30, 2024 13:30
Copy link
Contributor

@Martchus Martchus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it ran successfully:

lib/OpenQA/Worker/Isotovideo/Client.pm source OK
lib/perlcritic/Perl/Critic/Policy/HashKeyQuotes.pm source OK
shfmt -d -i 4 -bn -ci -sr $(shfmt -f .)
make test-unit-and-integration TIMEOUT_M=20 PROVE_ARGS="$HARNESS t/*{tidy,compile}*.t" GLOBIGNORE="t/25-cache-service.t:t/43-scheduling-and-worker-scalability.t:t/ui/01-list.t:t/ui/26-jobs_restart.t:t/ui/13-admin.t:"
make[1]: Entering directory '/home/squamata/project'

And the JavaScript check didn't complain anymore as well.

@mergify mergify bot merged commit 2770dd8 into os-autoinst:master Jan 30, 2024
41 checks passed
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.

5 participants