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

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

Integrate yamllint into project #25

Closed OlenaYefymenko closed 11 months ago

OlenaYefymenko commented 11 months ago

This patch provides a check to automatically ensure the correctness of syntax for YAML files in the project.

webknjaz commented 11 months ago

That style seems overly indented. Use the one with no extra indents for sequences. Also, pre-commit already runs in CI, integrating yamllint there means that it is executed already. No need to run it again.

OlenaYefymenko commented 11 months ago

That style seems overly indented. Use the one with no extra indents for sequences. Also, pre-commit already runs in CI, integrating yamllint there means that it is executed already. No need to run it again.

@webknjaz thank you, I edited this. As for separate yamllint in CI yes, it's not logical, I agree. I misunderstood your previous comment in 24 PR, sorry

webknjaz commented 11 months ago

@OlenaYefymenko it is customary to implement a way of hiding automated/formatting-only commits from the Git blame output. I want you to add a new separate commit with the .git-blame-ignore-revs file, following this example: https://github.com/sanitizers/octomachinery/blob/5609f56/.git-blame-ignore-revs.

Keep the comment with instructions but add your own commit hash. Do not perform any force-pushes after that since it'll change the commit has which is undesired.

This specific filename is recognized by GitHub and it'll also exclude listed commits from the blame view on web interface.

Once this is done, we'll merge this PR.

OlenaYefymenko commented 11 months ago

@OlenaYefymenko it is customary to implement a way of hiding automated/formatting-only commits from the Git blame output. I want you to add a new separate commit with the .git-blame-ignore-revs file, following this example: https://github.com/sanitizers/octomachinery/blob/5609f56/.git-blame-ignore-revs.

Keep the comment with instructions but add your own commit hash. Do not perform any force-pushes after that since it'll change the commit has which is undesired.

This specific filename is recognized by GitHub and it'll also exclude listed commits from the blame view on web interface.

Once this is done, we'll merge this PR.

@webknjaz it's interesting integration. I'd like to show you because I'm afraid that must execute force-push if I make a mistake. (Although I can change again hash)

# `git blame` master ignore list.
#
# This file contains a list of git hashes of revisions to be ignored
# by `git blame`. These revisions are considered "unimportant" in
# that they are unlikely to be what you are interested in when blaming.
# They are typically expected to be formatting-only changes.
#
# It can be used for `git blame` using `--ignore-revs-file` or by
# setting `blame.ignoreRevsFile` in the `git config`[1].
#
# Ignore these commits when reporting with blame. Calling
#
#   git blame --ignore-revs-file .git-blame-ignore-revs
#
# will tell `git blame` to ignore changes made by these revisions when
# assigning blame, as if the change never happened.
#
# You can enable this as a default for your local repository by
# running
#
#   git config blame.ignoreRevsFile .git-blame-ignore-revs
#
# This will probably be automatically picked by your IDE
# (VSCode+GitLens and JetBrains products are confirmed to do this).
#
# Important: if you are switching to a branch without this file,
# `git blame` will fail with an error.
#
# GitHub also excludes the commits listed below from its "Blame"
# views[2][3].
#
# [1]: https://git-scm.com/docs/git-blame#Documentation/git-blame.txt-blameignoreRevsFile
# [2]: https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/
# [3]: https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view
#
# Guidelines:
# - Only large (generally automated) reformatting or renaming PRs
#   should be added to this list. Do not put things here just because
#   you feel they are trivial or unimportant. If in doubt, do not put
#   it on this list.
# - When adding a single revision, use inline comment to link relevant
#   issue/PR. Alternatively, paste the commit title instead.
#   Example:
#     d4a8b7307acc2dc8a8833ccfa65426ad28b3ffc9  # https://github.com/sanitizers/octomachinery/issues/1
# - When adding multiple revisions (like a bulk of work over many
#   commits), organize them in blocks. Precede each such block with a
#   comment starting with the word "START", followed by a link to the
#   relevant issue or PR. Add a similar comment after the last block
#   line but use the word "END", followed by the same link.
#   Alternatively, add or augment the link with a text motivation and
#   description of work performed in each commit.
#   After each individual commit in the block, add an inline comment
#   with the commit title line.
#   Example:
#     # START https://github.com/sanitizers/octomachinery/issues/1
#     6f0bd2d8a1e6cd2e794cd39976e9756e0c85ac66  # Bulk-replace smile emojis with unicorns
#     d53974df11dbc22cbea9dc7dcbc9896c25979a27  # Replace double with single quotes
#     ... <rest of the list>
#     # END https://github.com/sanitizers/octomachinery/issues/1
# - Only put full 40-character hashes on this list (not short hashes
#   or any other revision reference).
# - Append to the bottom of the file, regardless of the chronological
#   order of the revisions. Revisions within blocks should be in
#   chronological order from oldest to newest.
# - Because you must use a hash, you need to append to this list in a
#   follow-up PR to the actual reformatting PR that you are trying to
#   ignore. This approach helps avoid issues with arbitrary rebases
#   and squashes while the pull request is in progress.

197f63ae0297da7bfa9dc4f65fdd739ce1621d83  # Fix formatting mistakes based on yamllint
OlenaYefymenko commented 11 months ago

I assume this commit should be added also Remove redundant type specification to avoid duplication instruction

webknjaz commented 11 months ago

I assume this commit should be added also Remove redundant type specification to avoid duplication instruction

No, this is not a formatting, not an automated change.

webknjaz commented 11 months ago

I'd like to show you because I'm afraid that must execute force-push if I make a mistake. (Although I can change again hash)

Technically, if you amend the last commit and make a force-push, that's fine for as long as you don't make changes that affect commit hashes.

The snippet is fine, I think.

That commit message for formatting is weird, though. The word "mistake" implies that it's something that is mandatory to fix because it's wrong somehow. But here, you just introduce a certain style that wasn't enforced earlier. It'd be a mistake if there was some style convention that you ignored. Since there was no such thing, it's not a mistake. And it's not a fix, since nothing was broken. You just adopt a new style and adjust the files to match it.

OlenaYefymenko commented 11 months ago

I'd like to show you because I'm afraid that must execute force-push if I make a mistake. (Although I can change again hash)

Technically, if you amend the last commit and make a force-push, that's fine for as long as you don't make changes that affect commit hashes.

The snippet is fine, I think.

That commit message for formatting is weird, though. The word "mistake" implies that it's something that is mandatory to fix because it's wrong somehow. But here, you just introduce a certain style that wasn't enforced earlier. It'd be a mistake if there was some style convention that you ignored. Since there was no such thing, it's not a mistake. And it's not a fix, since nothing was broken. You just adopt a new style and adjust the files to match it.

Thank you. Yes, I agree about the weird commit message, but I am confused about this change # yamllint disable-line rule: truthy. We can't say that it also adopts style. I don't want to be stuck for a long time on this issue, just want to share my hesitation. Also saw the article https://www.redhat.com/sysadmin/check-yaml-yamllint that if we talk about fixing due to yamllint we should use "error" like in other cases refer to scripts, not "mistake".

webknjaz commented 11 months ago

We can't say that it also adopts style.

Exceptions to rules aren't necessarily violations if they are justified. In this case, it's okay to allow this in one specific place, because another practice supercedes the linter rules. But we wouldn't normally want to allow this globally.

webknjaz commented 11 months ago

we should use "error" like in other cases refer to scripts, not "mistake"

"error" is just as ambiguous and it may be a correct term in some contexts. I prefer talking about "rule violations" because that's exactly what this is regardless of the surrounding context. Such violation might be a mistake, but it might be deliberate. If it's the latter, it's not an error, but a choice.

OlenaYefymenko commented 11 months ago

we should use "error" like in other cases refer to scripts, not "mistake"

"error" is just as ambiguous and it may be a correct term in some contexts. I prefer talking about "rule violations" because that's exactly what this is regardless of the surrounding context. Such violation might be a mistake, but it might be deliberate. If it's the latter, it's not an error, but a choice.

Now it's clear👍 I guess the last changes were committed.