kpi-web-guild / django-girls-blog-Martolivna

django-girls-blog-Martolivna created by GitHub Classroom
Other
0 stars 0 forks source link

Integrate pre-commit tool #15

Closed Martolivna closed 1 year ago

Martolivna commented 1 year ago

Add the following pre-commit hooks into the configuration file:

See more about pre-commit tool using.

Resolves #12

Martolivna commented 1 year ago

I have noticed some inaccuracy in the commit message and a missing period. Should I make a new PR?

webknjaz commented 1 year ago

No, use Git to edit the commit and force-push the branch. Also, apply the same rules to the PR title and description.

webknjaz commented 1 year ago

No, use Git to edit the commit and force-push the branch. Also, apply the same rules to the PR title and description.

While you're on it, see how you can link the related issue: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

webknjaz commented 1 year ago

@Martolivna I've taken a look at the new changes. flake8 is a linter, it does not format files so move it down. Also, some of your commit messages have a list of things in the title — this is a little hint that they are not atomic. If you can't come up with a commit title describing one thing, it's probably because it's actually two commits (or more). Still, the commit could be atomic within a branch but that level of detail wouldn't necessarily be useful later. Try to identify the related commits and squash them. This PR adds a new file and small fixups are probably going to be unnecessary outside of the PR. The atomic thing the PR does is adding a new config, that's useful to track globally. If you wanted, you could have separate hooks being added as separate commits which will end up on main. But thinks like adding and removing comments aren't going to add useful context to the history. Why add a commit that creates a broken config and a series of commits fixing that? You can use interactive rebase to reshape what the history would look like. This is a useful skill to have — while you're working on a PR and iterating on the feedback, it's okay to sometimes add a dummy commit or two, when you're debugging stuff, but then, it's good to make that look nice. Separate commits can be useful to demonstrate the path taken to reach the solution, or the flow of thought. Sometimes, even if they are perfectly atomic, you may want to inspect them and augment with extra detail for clarity. Also, while doing that, remember to add something like Resolves #12 because this is useful for the context and marking this in the PR, on the web UI, is not going to be reflected in Git which should have context that is useful even without the internet connection.

webknjaz commented 1 year ago

@Martolivna could you address this comment? https://github.com/kpi-web-guild/django-girls-blog-Martolivna/pull/15#issuecomment-1365597298

Also, while on it, I suggest dropping the separate pydocstyle hook in favor of flake8-docstrings. It needs to be integrated via additional_dependencies. Here's an example of how it's done: https://github.com/cherrypy/cheroot/blob/56d7b49/.pre-commit-config.yaml#L95.

webknjaz commented 1 year ago

@Martolivna could you also update the PR title and description?