Skip to content
This repository has been archived by the owner on Jan 26, 2021. It is now read-only.

Fixes #1035: Add black and flake8 as pre-commit hook #1058

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

BALaka-18
Copy link

@BALaka-18 BALaka-18 commented Aug 31, 2020

Description

This PR adds the code formatting tools, black and flake8, as pre-commit hook to ensure the code is organized and well formatted.

Fixes #1035

Type of Change:

  • Quality Assurance

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

  • Ran this pre commit hook on a demo repository of mine while trying to push python files.
  • All checks were executed successfully

Screenshots:

Screenshot (339)
Screenshot (340)

requirements.txt :

Screenshot (342)

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented my code or provided relevant documentation, particularly in hard-to-understand areas
  • Any dependent changes have been merged

Code/Quality Assurance Only

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@Kajol-Kumari
Copy link

@BALaka-18 Codewise LGTM! Can you please add the screenshot by runing this pre-commit hook on your local?

@BALaka-18
Copy link
Author

@BALaka-18 Codewise LGTM! Can you please add the screenshot by runing this pre-commit hook on your local?

Yes sure.

@BALaka-18
Copy link
Author

@Kajol-Kumari I have attached the screenshots.

@Kajol-Kumari
Copy link

Hey @BALaka-18 can we please add a check to insure that users install the pre-commit hook. Otherwise don't allow them to commit (reject the commit).

Copy link

@SanketDG SanketDG left a comment

Choose a reason for hiding this comment

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

Looks good from a first look, can you post the diff of requirements.txt, I can't seem to see it in the view for some reason

@BALaka-18
Copy link
Author

BALaka-18 commented Sep 1, 2020

Looks good from a first look, can you post the diff of requirements.txt, I can't seem to see it in the view for some reason

@SanketDG I had removed the older versions of six and PyYAML, else the build was running into errors.
Otherwise all same.

@BALaka-18
Copy link
Author

@Kajol-Kumari I have added the check.

Copy link

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

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

@BALaka-18 Minor changes required :)

  1. Add a ss of requirements.txt file in issue description.
  2. Incorporate the suggested change.

pre-commit_documentation.md Outdated Show resolved Hide resolved
@Kajol-Kumari
Copy link

Hey @BALaka-18 If I am not wrong, you have just pasted the ss of the changes You have done in requirement.txt file and not the whole file content? (just want to confirm)

@BALaka-18
Copy link
Author

Hey @BALaka-18 If I am not wrong, you have just pasted the ss of the changes You have done in requirement.txt file and not the whole file content? (just want to confirm)

@Kajol-Kumari these are the extra dependencies required to make the pre-commit hook run. The rest of the dependencies are the same as in the current requirements.txt file. So I passed only the new dependencies needed and not the whole file.

Should I upload the ss for the whole file ?

@Kajol-Kumari
Copy link

Thanks for confirming @BALaka-18. No, you don't need to attach the ss of the whole file here.

Kajol-Kumari
Kajol-Kumari previously approved these changes Sep 2, 2020
Copy link

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@BALaka-18
Copy link
Author

Hi @Kajol-Kumari, is this PR ready to be merged ?
Could you please request a review from a reviewer with write access ? It still says that review is required.

@Kajol-Kumari
Copy link

@BALaka-18 please squash all the commits into a single commit so that it can be send for final approval.

@BALaka-18
Copy link
Author

@BALaka-18 please squash all the commits into a single commit so that it can be send for final approval.

@Kajol-Kumari done !

@Kajol-Kumari Kajol-Kumari requested review from SanketDG and Kajol-Kumari and removed request for Kajol-Kumari September 5, 2020 16:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] Use 'black' and 'fake8' as pre-commit hook
3 participants