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

django-girls-blog-slsh1o created by GitHub Classroom
MIT License
0 stars 0 forks source link

Configure pre-commit tool #17

Closed slsh1o closed 4 years ago

slsh1o commented 4 years ago

All tests passed and everything looks fine. Check this out please.

Also ask for review @dark-lord671 and @ZVitaliy .

Close #14

webknjaz commented 4 years ago

I think the commit messages can be improved:

Configure pre-commit file

There's no such thing as a pre-commit file. You're adding a config for the tool called pre-commit.

Fix end of files

This isn't precise. The commit message should communicate the intent behind the change, not the implementation detail. It should answer to "why?" because "how?" is already answered in the change itself. Also, claiming that it fixes something implies that it's broken but it's not clear in what way. I'd go for "normalize" and maybe add a proper long description of what that actually means.

Please improve the commit messages in accordance with #6/#7. Use an interactive rebase to achieve this.

slsh1o commented 4 years ago

Now commits messages should be more precise. Looks like I accidentally added dot instead of 's' to the 712c588 commit description. Hope it's not such a big problem 😅

webknjaz commented 4 years ago

blank lines

There's no such term. You added a line feed (aka LF, aka 0x0A, aka \n)

webknjaz commented 4 years ago

A few files doesn't have blank lines on the file end and don't pass pre-commit test.

This does not describe what is the effect of applying the commit. It only points to some behavior that seems to be happening under some undefined conditions and it's not clear if that's something that was happening before the commit or it started happening as a result of applying it.

slsh1o commented 4 years ago

This time I tried to answer why I did it. I understand that 'blank line' isn't a term but still. In commit 'title' I described what I did and than in description why.

If it will look like so:

Add line feed to the files end

From your comment in #18 I now understand why I should mention files names but than in cases like that it will make commit 'title' too long. Should I put all changed files in description? But it will still require to check this commit content, though😅

And then in description:

Fixed files doesn't have line feeds on the file end and don't pass "Fix End of Files" pre-commit test

will it ok? Or I don't understand something?

webknjaz commented 4 years ago

This time I tried to answer why I did it.

The commit message should first point out what would be the effect of applying this change to the project. And then, if needed, go into detail of what inspired the change, sometimes explaining the problem and possible solutions, even reasons behind rejecting some of the options and what are the alternatives. But that's for big or not very obvious changes. You should try to understand how this would look to the reader.

Here's what I'd write:

Normalize file endings to include LF

This change makes sure that all the text files in the project
have line feed byte at the end. It's necessary to please a
newly integrated linter tool — pre-commit — that enforces such
standard.

Normally, I'd probably omit the long description part for such a small change but for the sake of exercise, it's good to practice writing good commit messages.

slsh1o commented 4 years ago

I definitely need more practice with commit messages :D Now it look more clear, thx.

slsh1o commented 4 years ago

Oh and should I rewrite message for 54d6e6a ? Looks like it clear enough for now.

webknjaz commented 4 years ago

Yep, rewrite that too. Also,

Add pre commit checks w/ pre-commit

suggests that this commit will somehow automagically configure pre-commit hooks in the Git repo which isn't true: it only adds a configuration file for a tool which users can then optionally enable to also run as their pre-commit script.

webknjaz commented 4 years ago

Configure pre-commit tool to exclude some basic errors

Well, linters don't exclude errors. Errors still need to be fixed by humans. But they guard against adding new ones, in a way.