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 pre commit for python linter and formatter #259

Merged
merged 25 commits into from
Jun 4, 2022
Merged

Add pre commit for python linter and formatter #259

merged 25 commits into from
Jun 4, 2022

Conversation

Enzyme3
Copy link
Contributor

@Enzyme3 Enzyme3 commented May 20, 2022

Fixes #258

Changes

  • Added a pre-commit hook
  • Added app/.pre-commit-config.yaml which:
    • formats with isort on python files
    • formats with black on python files
    • lints with flake8 on python files
    • lints with pytlint (with pylint-django plugin) on python files
      • ignores the mkdocs directory
    • formats miscellaneous things like whitespaces and EOF for all files
  • Applied the formatting and addressed the linting errors on the existing codebase

Lots of files were modified due to the formatting/linting but those were purely cosmetic. Should focus more on:

  • app/.pre-commit-config.yaml
  • app/setup.cfg

@Enzyme3 Enzyme3 requested a review from Aveline-art May 20, 2022 04:29
@Enzyme3
Copy link
Contributor Author

Enzyme3 commented May 20, 2022

Lint error is: /github/workspace/dev/webpack.dockerfile:1 DL3007 warning: Using latest is prone to errors if the image will ever update. Pin the version explicitly to a release tag

the formatter removed some whitespace and it looks like changing the file pulled it in for validation by super-linter. It was originally checked in as node:latest and that has not changed. LMK if you want to just suppress it or pin it to a particular version

@Enzyme3 Enzyme3 mentioned this pull request May 20, 2022
3 tasks
@Enzyme3
Copy link
Contributor Author

Enzyme3 commented May 20, 2022

added README.md under app on how to setup linter/formatter

Copy link
Member

@Aveline-art Aveline-art left a comment

Choose a reason for hiding this comment

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

Hi @Enzyme3 . Great work on this as always.

A few more notes:

  • The readme should be placed inside of our installation instructions which would help ensure our documentation is all in the same place rather than in multiple places at once.
  • For the lint error, node:18 is fine as far as I can tell.
  • If these changes are not critical for setting up our staging env, let's merge this into our dev-branch-1 branch to avoid creating too many merge conflicts. Also it looks like your working branch has not been updated in a while, just fyi :)

app/README.md Outdated Show resolved Hide resolved
@Enzyme3 Enzyme3 changed the base branch from main to dev-branch-1 May 21, 2022 18:59
Copy link
Member

@Aveline-art Aveline-art left a comment

Choose a reason for hiding this comment

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

Hi @Enzyme3 . Thank you for waiting for so long. I had to wait for the issue to be approved before merging. Everything looks perfect! Just a few small changes:

  • Let's move the .env file into the dev/ directory to keep the env with the related dockerfiles. Likewise, we can also rename the file to linter.env or some such. Make sure that linter.dockerfile still works
  • Since we have linted those files, let's take out these lines from linter.yml
  VALIDATE_PYTHON: false
  VALIDATE_PYTHON_BLACK: false
  VALIDATE_PYTHON_FLAKE8: false
  VALIDATE_PYTHON_ISORT: false
  VALIDATE_PYTHON_MYPY: false
  VALIDATE_PYTHON_PYLINT: false

Once that is done, feel free to merge it at your leisure.

@Enzyme3
Copy link
Contributor Author

Enzyme3 commented Jun 4, 2022

Thanks @Aveline-art , have renamed the .env file and placed it in dev
I did not take out the python references in the linter.yml yet because we probably still want to have the linter auto-execute everytime the PR gets created.

Perhaps another TODO can be raised to enable it, and ensure that the local linting rules match with the github one

@Enzyme3 Enzyme3 merged commit 0abea87 into hackforla:dev-branch-1 Jun 4, 2022
@Enzyme3 Enzyme3 deleted the add-pre-commit-for-python-linter-and-formatter branch June 4, 2022 18:54
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.

Setup linter / formatter for python
2 participants