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

Always run the spell and shell checkers if possible, print warning when not possible #54847

Merged
merged 1 commit into from Oct 9, 2023

Conversation

strk
Copy link
Contributor

@strk strk commented Oct 5, 2023

I was annoyed by the CI bot failing due to spellchecker (US vs. UK english) and wondered why the pre-commit hook was not running this locally, then found out I had to set an environment variable to enable that. This commit is to always run the spell checker when requirements are met and print a WARNING when they aren't, so that developers have more probabilities to not disappoint the bot lords

@github-actions github-actions bot added this to the 3.34.0 milestone Oct 5, 2023
@strk
Copy link
Contributor Author

strk commented Oct 5, 2023

Background story: #54827 (comment)

@strk
Copy link
Contributor Author

strk commented Oct 5, 2023

Silly me, I tried to sneak in a funny joke, didn't think bots were also lacking irony.
I replaced the while :; do ...; break; done construct with a for us in pain; do ...; done but:

^-^ SC2034 (warning): us appears unused. Verify use (or export if used externally).

@strk
Copy link
Contributor Author

strk commented Oct 5, 2023

shell cheker is another one that isn't runnig for me as pre-commit hook

@strk
Copy link
Contributor Author

strk commented Oct 5, 2023

Now, as I really WANT a single-iteration loop, I could put the previous construct back but I'd be fighting against the shellcheck bot:
https://www.shellcheck.net/wiki/SC2043

@strk
Copy link
Contributor Author

strk commented Oct 5, 2023

I've filed the issue upstream to koalaman/shellcheck#2839

@strk strk changed the title Always run the spellchecker, print warning if requirements not met Always run the spell and shell checkers if possible, print warning when not possible Oct 5, 2023
scripts/prepare_commit.sh Outdated Show resolved Hide resolved
done
fi

if [[ -x "${TOPLEVEL}"/tests/code_layout/test_shellcheck.sh ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not worth checking this, if it's ever dropped, it's a good idea to update this script too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually agree, but I don't know why the original author added that check (same can be said on line 60, where check_spelling is chekced)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to not check anymore

@strk strk added the Chore GitHub and other CI infrastructure changes label Oct 6, 2023
scripts/prepare_commit.sh Outdated Show resolved Hide resolved
@strk strk merged commit fe1d167 into qgis:master Oct 9, 2023
28 checks passed
@troopa81
Copy link
Contributor

troopa81 commented Oct 9, 2023

@strk it looks like it runs on every shell files existing in the QGIS repository. So, on my side, it runs on all files from different worktrees and slow down considerably the commit process and would eventually fail on files that I have no intention to push.

Could we apply it only on modified files, like it's done for spell check ?

@3nids
Copy link
Member

3nids commented Oct 9, 2023

same here, even git ignored files.

@3nids
Copy link
Member

3nids commented Oct 9, 2023

also, running the spell check everytime makes the prepare_commit quite slow (painful when using it for sip modifications).
we could maybe add an argument for this?

@strk strk deleted the spellchecker branch October 9, 2023 13:00
@kannes
Copy link
Contributor

kannes commented Oct 9, 2023

also, running the spell check everytime makes the prepare_commit quite slow (painful when using it for sip modifications). we could maybe add an argument for this?

As a very casual contributor of small things I would plead for keeping this as "the default that will make commits much more likely to not put the contributor into another slow feedback loop of failing tests". Maybe disabling it could be the option?

@strk
Copy link
Contributor Author

strk commented Oct 9, 2023

See GH-54893 for speed improvements

@strk
Copy link
Contributor Author

strk commented Oct 9, 2023

on my side, it runs on all files from different worktrees

This should be fixed by GH-54893 but out of curiosity: is this happening because you have all worktrees under the parent directory ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Chore GitHub and other CI infrastructure changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants