Skip to content

improve linting#1820

Merged
Frenzie merged 5 commits into
koreader:masterfrom
benoit-pierre:pr/shellcheck
Jun 13, 2024
Merged

improve linting#1820
Frenzie merged 5 commits into
koreader:masterfrom
benoit-pierre:pr/shellcheck

Conversation

@benoit-pierre

@benoit-pierre benoit-pierre commented Jun 11, 2024

Copy link
Copy Markdown
Member

The plan is to use the new ./utils/shellcheck.sh helper in koreader too, to avoid duplication and ensure the same standard is used.


This change is Reviewable

- ensure shellcheck behavior is not affected by the
  presence of a `.shellcheckrc` in a parent directory
- leave the terminal attributes to default at exit
- use the same standard as koreader
- leave the terminal attributes to default at exit
- drop dead code
- fix lint issues
- do run luacheck even if there are shellcheck issues
- always run luacheck directly: so a luacheck install for
  another LUA version or using a shell script launcher
  works (the docker image wrapper already uses luajit)
- don't disable colors in luacheck output
Comment thread .ci/helper_shellchecks.sh
Comment on lines -10 to -27
SHELLSCRIPT_ERROR=0

for shellscript in "${shellscript_locations[@]}"; do
echo -e "${ANSI_GREEN}Running shellcheck on ${shellscript}"
shellcheck "${shellscript}" || SHELLSCRIPT_ERROR=1
echo -e "${ANSI_GREEN}Running shfmt on ${shellscript}"
if ! shfmt -i 4 "${shellscript}" >/dev/null 2>&1; then
echo -e "${ANSI_RED}Warning: ${shellscript} contains the following problem:"
shfmt -i 4 "${shellscript}" || SHELLSCRIPT_ERROR=1
continue
fi
if [ "$(cat "${shellscript}")" != "$(shfmt -i 4 "${shellscript}")" ]; then
echo -e "${ANSI_RED}Warning: ${shellscript} does not abide by coding style, diff for expected style:"
shfmt -i 4 "${shellscript}" | diff "${shellscript}" - || SHELLSCRIPT_ERROR=1
fi
done

exit "${SHELLSCRIPT_ERROR}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's worth noting that if there were any improvements since this was written, it's this file that would've been potentially overlooked and not the one in front.

As it happens that one was changed to use a variable, albeit combined with an ugly shellcheck disable.

koreader/koreader@cb2314d

More important because it affects the output, I added -ci (switch cases will be indented) in koreader/koreader@668eee9

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Although the commit "utils: lint" at fc9c8aa seems to suggest -ci may be the default now?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah never mind, I somehow accidentally skipped past a commit.

Let me paraphrase in any case as that as structured I find it harder to gauge what are changes and what's merely updating it as per in front, but it looks like it's carrying over the most important changes.

@Frenzie Frenzie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

PS Mostly just thinking aloud, no need to change anything in my book.

@Frenzie Frenzie merged commit 8d61ff0 into koreader:master Jun 13, 2024
@benoit-pierre benoit-pierre deleted the pr/shellcheck branch June 13, 2024 19:21
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