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

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

Update linters configuration #24

Closed OlenaYefymenko closed 11 months ago

OlenaYefymenko commented 11 months ago

Resolves #23

webknjaz commented 11 months ago

Edit pre-commit configuration for correct operation of the linters;

It's rather obvious that commits would contain file edits to achieve something. But it'd be better to write down what the motivation and the effect of the changes is. Saying "Do something so it's done" doesn't provide useful or accurate details. Add some. Also, define what's "proper operation" and what was improper about the previous state of it.

webknjaz commented 11 months ago

Fix warning raised by yamllint in the CI workflow configuration

What warning? What's fixed? How? Why?

webknjaz commented 11 months ago

Update linters configuration

This title is very broad and doesn't accurately refer to anything useful. Rephrase it to say one exact thing the PR is supposed to achieve.

OlenaYefymenko commented 11 months ago

Update linters configuration

This title is very broad and doesn't accurately refer to anything useful. Rephrase it to say one exact thing the PR is supposed to achieve.

@webknjaz ok, I will try. But should I avoid specifying the exact filename? Did I understand correctly (I remember this recommendation from previous PR?)

webknjaz commented 11 months ago

But should I avoid specifying the exact filename?

It's usually unnecessary, unless how the file is named is an important detail. This probably depends on what you want the PR to be about. What's one end-goal? Is it about renaming a file? Is it about configuring checks/linters in some way? Is it important enough to be in the title as opposed to the long description?

Did I understand correctly (I remember this recommendation from previous PR?)

This depends on whether it's an important detail that would improve accuracy of the title while not referring to unimportant low-level implementation details. The title is supposed to be high-level and accurate. If I get email notification about 5 PRs saying "update linters configuration", I'm not going to understand what's that all about. It's not compelling to open each of such notifications and wrestle the mind with what's the difference between those might be.

webknjaz commented 11 months ago

Why did you close this?

webknjaz commented 11 months ago

You could probably repurpose the PR to make changes related to pylint, or docstring linting, or version updates.

OlenaYefymenko commented 11 months ago

Why did you close this?

Because, name of branch already not suit to current issue (pylint), I deleted old branch. Also, a lot of not relevant commits.

webknjaz commented 11 months ago

Well, most people don't look into the branch name anyway. So it's usually acceptable to iterate on things. The context is in the comments, by losing the context, you now have to keep track of two locations to react to things asked elsewhere in your new PR. I'd say this introduces unnecessary cognitive load, a mental overhead that could've been avoided.

webknjaz commented 11 months ago

By the way, it's possible to reopen PRs after having resurrected the branch to its original state (name + the same commit at head).

OlenaYefymenko commented 11 months ago

By the way, it's possible to reopen PRs after having resurrected the branch to its original state (name + the same commit at head).

@webknjaz ok. I'll reopen for flake8 Also, I noticed in your repo some useful settings: https://github.com/cherrypy/cheroot/blob/00c3cc3e5331fbaa10c44e5080899f1f3d9da6e7/.flake8 Can I focus on this example for my project? I mean what understandable to me

OlenaYefymenko commented 11 months ago

By the way, it's possible to reopen PRs after having resurrected the branch to its original state (name + the same commit at head).

I tried, but couldn't reopen it. Additionally, I need the latest changes from the main branch, which pertain to edits in YAML files.

webknjaz commented 11 months ago

You can reopen it only if you recover the exact branch state with the same head commit.