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

chore: Add pre-commit guide in README #6991

Merged
merged 10 commits into from
Aug 2, 2021

Conversation

salonigupta1
Copy link
Contributor

Fixes #6862

Short description of what this resolves:

A quick guide to configure pre-commits on Local Installation.

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

@auto-label auto-label bot added the chore label May 11, 2020
@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #6991 (d0383e5) into development (42ff39c) will decrease coverage by 5.99%.
The diff coverage is n/a.

❗ Current head d0383e5 differs from pull request most recent head 1f72b45. Consider uploading reports for the commit 1f72b45 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6991      +/-   ##
===============================================
- Coverage        66.55%   60.55%   -6.00%     
===============================================
  Files              303      259      -44     
  Lines            15499    12872    -2627     
===============================================
- Hits             10315     7795    -2520     
+ Misses            5184     5077     -107     
Impacted Files Coverage Δ
app/api/custom/orders.py 19.17% <0.00%> (-48.88%) ⬇️
app/api/helpers/query.py 43.47% <0.00%> (-48.53%) ⬇️
app/api/helpers/permission_manager.py 28.77% <0.00%> (-43.33%) ⬇️
app/api/events.py 21.70% <0.00%> (-31.87%) ⬇️
app/api/speakers.py 47.42% <0.00%> (-28.04%) ⬇️
app/api/helpers/validations.py 50.00% <0.00%> (-25.00%) ⬇️
app/api/sessions.py 41.46% <0.00%> (-20.42%) ⬇️
app/api/orders.py 26.86% <0.00%> (-19.79%) ⬇️
app/api/schema/session_types.py 82.60% <0.00%> (-17.40%) ⬇️
app/api/helpers/scheduled_jobs.py 48.29% <0.00%> (-16.71%) ⬇️
... and 212 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42ff39c...1f72b45. Read the comment docs.

Copy link
Member

@iamareebjamal iamareebjamal left a comment

Choose a reason for hiding this comment

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

Invalid Instructions

README.md Outdated
Comment on lines 275 to 292
**Introduction**

Git hook scripts are useful for identifying simple issues before submission to code review. We run our hooks on every commit to automatically point out issues in code such as missing semicolons, trailing whitespace, and debug statements. By pointing these issues out before code review, this allows a code reviewer to focus on the architecture of a change while not wasting time with trivial style nitpicks.

### Quick Start

#### 1. Add a pre-commit configuration ¶

* create a file named .pre-commit-config.yaml
* you can generate a very basic configuration using pre-commit sample-config

#### 2. Install the git hook scripts ¶
* run pre-commit install to set up the git hook scripts
```sh
$ pre-commit install
pre-commit installed at .git/hooks/pre-commit
```
* now pre-commit will run automatically on git commit!
Copy link
Member

Choose a reason for hiding this comment

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

You need to document about configuring. This is for Setup, which is already there.

Copy link
Member

Choose a reason for hiding this comment

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

Even for documenting configuration, I think pasting the link https://pre-commit.com is enough. @iamareebjamal What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We should still document pre-commit install

README.md Outdated
Comment on lines 273 to 279
## Pre-commits guide

**Introduction**

Git hook scripts are useful for identifying simple issues before submission to code review. We run our hooks on every commit to automatically point out issues in code such as missing semicolons, trailing whitespace, and debug statements. By pointing these issues out before code review, this allows a code reviewer to focus on the architecture of a change while not wasting time with trivial style nitpicks.

### For installation and configuration, [Click Here](https://pre-commit.com/)
Copy link
Member

Choose a reason for hiding this comment

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

#6991 (comment).
Plus, I don't think this is a good place in the documentation for the Pre-commits guide. Include it in the development section.

@iamareebjamal
Copy link
Member

Hi, let me re-iterate what is the requirement of this PR.
We want people who are doing local development to run pre-commit install after they have cloned the project, create virtualenv and installed dependencies

That's it

README.md Outdated
Comment on lines 260 to 264

**Introduction**

Git hook scripts are useful for identifying simple issues before submission to code review. We run our hooks on every commit to automatically point out issues in code such as missing semicolons, trailing whitespace, and debug statements. By pointing these issues out before code review, this allows a code reviewer to focus on the architecture of a change while not wasting time with trivial style nitpicks.

Copy link
Member

Choose a reason for hiding this comment

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

No need to add the introduction as well.

@maze-runnar maze-runnar merged commit 480e662 into fossasia:development Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document about configuring pre-commit on local installation
4 participants