Skip to content

Add pep8-naming and more pre-commit hooks#64

Merged
jb3 merged 8 commits into
masterfrom
feat/deps/63/pep8-naming
Jan 10, 2021
Merged

Add pep8-naming and more pre-commit hooks#64
jb3 merged 8 commits into
masterfrom
feat/deps/63/pep8-naming

Conversation

@MarkKoz
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz commented Mar 5, 2020

Relevant Issues

Resolves #63
python-discord/organisation#138

Description

New hooks were added for pre-commit and they will run in CI too. The pipenv run lint script will now run all the new hooks, including flake8.

Hooks added

A couple of these hooks automatically apply fixes. However, they still report failure and leave any changes they make uncommitted. Therefore, the user has to commit the automatic fixes.

  • check-merge-conflict - Check for files that contain merge conflict strings.
  • check-toml - Attempts to load all toml files to verify syntax.
  • check-yaml - Attempts to load all yaml files to verify syntax.
  • end-of-file-fixer - Makes sure files end in a newline and only a newline.
  • mixed-line-ending - Replaces mixed line endings with LF.
  • trailing-whitespace - Trims trailing whitespace.
  • python-check-blanket-noqa - Enforce that noqa annotations always occur with specific codes

Reasoning

  • Dev dependencies were updated and re-pinned because why not? They were getting old enough. I'm just following along with the other repos.
  • No cache used for pre-commit because it runs in a Docker container. I suppose it could be copied to the container from the cache on the host, but that doesn't seem worthwhile.

@MarkKoz MarkKoz added type: feature New feature or request area: dependencies Related to package dependencies and management status: needs review Author is waiting for someone to review and approve area: CI Related to continuous intergration and deployment priority: 2 - normal labels Mar 5, 2020
@MarkKoz MarkKoz requested a review from a team as a code owner March 5, 2020 05:51
@MarkKoz MarkKoz requested review from MrHemlock and jerbob March 5, 2020 05:51
@MarkKoz
Copy link
Copy Markdown
Contributor Author

MarkKoz commented Mar 5, 2020

pre-commit fails to run cause git is not installed in the container. May need to rethink this...

@MarkKoz MarkKoz added status: WIP Work in progress and removed status: needs review Author is waiting for someone to review and approve labels Mar 5, 2020
@MarkKoz MarkKoz force-pushed the feat/deps/63/pep8-naming branch from 84e39d9 to 900dd5f Compare March 23, 2020 19:54
@ghost ghost added the needs 1 approval label Jul 10, 2020
@MarkKoz MarkKoz force-pushed the feat/deps/63/pep8-naming branch 2 times, most recently from a0386a1 to ee9090f Compare August 19, 2020 22:28
@MarkKoz
Copy link
Copy Markdown
Contributor Author

MarkKoz commented Aug 19, 2020

Build failing because pre-commit cannot find shellcheck

Test shell scripts with shellcheck.......................................Failed

  • hook id: shellcheck
  • exit code: 127

/root/.cache/pre-commit/repo_gx8xowx/pre_commit_hooks/shellcheck: 13: /root/.cache/pre-commit/repo_gx8xowx/pre_commit_hooks/shellcheck: shellcheck: not found

@MarkKoz MarkKoz marked this pull request as draft August 19, 2020 22:40
@MarkKoz
Copy link
Copy Markdown
Contributor Author

MarkKoz commented Aug 19, 2020

Now I don't know why it fails. Maybe cause it's an older version (0.5.0). Shellcheck 0.7.1 doesn't complain locally.

Test shell scripts with shellcheck.......................................Failed
- hook id: shellcheck
- exit code: 1

In scripts/check_dockerfiles.sh line 22:
    if [[ -v build_cache["${branch}"] ]]; then
    ^-- SC1009: The mentioned syntax error was in this if expression.
       ^-- SC1073: Couldn't parse this test expression. Fix to allow more checks.
             ^-- SC1019: Expected this to be an argument to the unary condition.
                                     ^-- SC1020: You need a space before the ]].
                                     ^-- SC1072: Missing space before ]. Fix any mentioned problems and try again.


In scripts/dev.sh line 17:
    && printf " done!\n" || exit "$?"
                     ^-- SC1117: Backslash is literal in "\n". Prefer explicit escaping: "\\n".


In scripts/dev.sh line 28:
            && printf " done!\n" || exit "$?"
                             ^-- SC1117: Backslash is literal in "\n". Prefer explicit escaping: "\\n".

@MarkKoz MarkKoz force-pushed the feat/deps/63/pep8-naming branch from 7329d4a to f10bf29 Compare August 20, 2020 00:41
@MarkKoz
Copy link
Copy Markdown
Contributor Author

MarkKoz commented Aug 20, 2020

Okay, I give up on ShellCheck. I can't install from the tarball cause the container doesn't have xz to decompress it. I could install xz but this just isn't worth it any more. I should honestly rewrite the shell scripts in Python anyway.

@MarkKoz MarkKoz force-pushed the feat/deps/63/pep8-naming branch from f10bf29 to 9a493dc Compare August 20, 2020 00:49
@MarkKoz MarkKoz marked this pull request as ready for review August 20, 2020 00:53
@ghost ghost added the needs 2 approvals label Aug 20, 2020
@MarkKoz MarkKoz removed the status: WIP Work in progress label Aug 20, 2020
@ghost ghost removed the needs 2 approvals label Aug 20, 2020
@MarkKoz MarkKoz mentioned this pull request Nov 14, 2020
@jb3 jb3 removed the request for review from a team November 28, 2020 02:47
@MarkKoz MarkKoz requested a review from jb3 as a code owner January 8, 2021 22:58
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 8, 2021

Coverage Status

Coverage remained the same at 93.421% when pulling e8e27a9 on feat/deps/63/pep8-naming into 9ff15de on master.

@MarkKoz
Copy link
Copy Markdown
Contributor Author

MarkKoz commented Jan 9, 2021

@Xithrius Are you planning on finishing this? If I remember correctly, @SebastiaanZ was since he did #79 but I think he's been too busy. If you are, then the CI stuff here still needs to be migrated to GH Actions. Otherwise, I should find time to finish this myself since it is my PR after all.

By the way, why are you putting "rebase" in your commit summaries when they look like they are merge commits, not rebases? If you were to actually rebase master, that would have required a force push.

@Xithrius
Copy link
Copy Markdown
Contributor

Xithrius commented Jan 9, 2021

@Xithrius Are you planning on finishing this?

I don't know how to properly test this PR, so it would probably be best for you to finish this up.

By the way, why are you putting "rebase" in your commit summaries when they look like they are merge commits, not rebases?

I've accidentally used the wrong terminology, thank you for pointing that out.

@MarkKoz MarkKoz marked this pull request as draft January 9, 2021 06:38
@MarkKoz MarkKoz force-pushed the feat/deps/63/pep8-naming branch 3 times, most recently from c5e99a7 to ae8a5f0 Compare January 9, 2021 22:02
@MarkKoz MarkKoz marked this pull request as ready for review January 9, 2021 22:24
It is a flake8 plugin which enforces PEP 8 naming conventions.

Resolves #63
Hooks added:

* check-merge-conflict - checks for files with merge conflict strings
* check-toml - attempts to load all toml files to verify syntax
* check-yaml - attempts to load all yaml files to verify syntax
* end-of-file-fixer - ensures files end in a newline and only a newline
* mixed-line-ending - replaces mixed line endings with LF
* trailing-whitespace - trims trailing whitespace
* python-check-blanket-noqa - enforces that noqa annotations always
  occur with specific codes

See: python-discord/organisation#138
Pre-commit requires git.
@MarkKoz MarkKoz force-pushed the feat/deps/63/pep8-naming branch from cd5ec15 to 720a32f Compare January 10, 2021 03:18
Copy link
Copy Markdown
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

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

This all looks good. Everything works perfect locally.

@jb3 jb3 merged commit 7e6000e into master Jan 10, 2021
@jb3 jb3 deleted the feat/deps/63/pep8-naming branch January 10, 2021 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: CI Related to continuous intergration and deployment area: dependencies Related to package dependencies and management needs 1 approval priority: 2 - normal type: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pep8-naming to the linting toolchain.

4 participants