Skip to content

Feature/fixup pylint example errors #6

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Feature/fixup pylint example errors #6

wants to merge 3 commits into from

Conversation

hugodahl
Copy link

This add calls to pylint through the pre-commit hooks, instead of the GitHub workflow.

This is a draft PR until the GitHub workflow actions in build.yml are also updated.

@hugodahl
Copy link
Author

For now, both calls to pylint are done through shell commands, so that the changes could be made as quickly as possible to make it work.

Ultimately, at the very least, the checks for library files, adafruit_*.py, should run using the existing pre-commit hook for pylint from the repo at "https://github.com/pycqa/pylint". See issue #104 in the Cookiecutter repository.

gamblor21
gamblor21 approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@gamblor21 gamblor21 left a comment

Choose a reason for hiding this comment

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

Checked the actions output. pylint ran as part of pre-commit and not its own action.

kattni
kattni suggested changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

Thank you so much for doing this! Let's hold off on this PR though, and get it into cookiecutter first if this is how it should be done. We'll do a patch of all the libraries after that. Please see my question below.

.pre-commit-config.yaml
@@ -17,3 +17,17 @@ repos:
- id: check-yaml
- id: end-of-file-fixer
- id: trailing-whitespace
- repo: local
Copy link
Contributor

Choose a reason for hiding this comment

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

Please explain how this works. I didn't look into it too deeply, but my first impression was that local meant local to your machine. Will this still work in the library when someone PRs to it?

Copy link
Author

Choose a reason for hiding this comment

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

Right, it means that it's not pulling the code that pre-commit runs from another repo, but rather, it's executing things locally.

Since part of the GitHub workflow is to install pylint, and that happens before pre-commit runs when someone creates a PR against a GitHub repository, we will be assured that pylint is installed.

The downside is that pylint isn't automatically installed as it would with pre-commit. If we want, we could add that to the custom hook, however, I know we'd rather not pollute peoples' environments by installing things willy-nilly.

@hugodahl
Copy link
Author

Closing this issue since the root fix is addressed with PR #109 in the Cookiecutter repo.

@hugodahl hugodahl closed this Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kattni kattni kattni requested changes

@gamblor21 gamblor21 gamblor21 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hugodahl @kattni @gamblor21