Skip to content
Snippets Groups Projects

Enable HAML-LINT linters

Merged Enable HAML-LINT linters
22072-enable-haml-lints into master
All threads resolved!
Merged Kushal Pandya requested to merge 22072-enable-haml-lints into master
All threads resolved!

What does this MR do?

This MR enables new HAML Linters as mentioned in #22072 (closed) (particularly in this comment)

  • AltText
  • ClassAttributeWithStaticValue
  • FinalNewline
  • HtmlAttributes
  • ImplicitDiv
  • SpaceBeforeScript
  • TrailingWhitespace
  • SpaceInsideHashAttributes

Are there points in the code the reviewer needs to double check?

Pretty much every change needs to be reviewed as we want to be sure that an attempt to fix a Lint rule doesn't cause any unintended side-effect (which otherwise doesn't happen)

Why was this MR needed?

See #22072 (closed) and discussion thread in same issue.

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #22072 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Kushal Pandya added 41 commits

    added 41 commits

    • fe3c8385...796b5b57 - 24 commits from branch master
    • ca7cf15e - HAMLLint: Enable AltText rule
    • 35a17436 - HAMLLint: fix AltText offences
    • a8d9254e - HAMLLint: Enable ClassAttributeWithStaticValue rule
    • 7e88b8f4 - HAMLLint: Fix ClassAttributeWithStaticValue offences
    • 579b11e4 - HAMLLint: Enable FinalNewline rule
    • 8b9e7e05 - HAMLLint: Fix FinalNewline offences
    • 0ca881f0 - HAMLLint: Enable HtmlAttributes rule
    • faf1f445 - HAMLLint: Fix HtmlAttributes offences
    • 851a61d2 - HAMLLint: Enable ImplicitDiv rule
    • 080afcd5 - HAMLLint: Fix ImplicitDiv offences
    • d422718b - HAMLLint: Enable SpaceBeforeScript rule
    • b00b471b - HAMLLint: Fix SpaceBeforeScript offences
    • be77027e - HAMLLint: Enable TrailingWhitespace rule
    • 2504c902 - HAMLLint: Fix TrailingWhitespace offences
    • 26ae1e03 - HAMLLint: Enable SpaceInsideHashAttributes rule
    • 0c44ee1a - HAMLLint: Fix SpaceInsideHashAttributes offences
    • 3802abf7 - HAMLLint: Fix SpaceInsideHashAttributes offence

    Compare with previous version

    Toggle commit list
  • Kushal Pandya resolved all discussions

    resolved all discussions

  • Alfredo Sumaran
    Alfredo Sumaran @asumaran ·
    Contributor

    Are we OK with merging this? @rspeicher @jschatz1

  • Robert Speicher
    Robert Speicher @rspeicher ·
    Contributor

    In theory. I'm wondering two things:

    1. Considering the "Pretty much every change needs to be reviewed as we want to be sure that an attempt to fix a Lint rule doesn't cause any unintended side-effect (which otherwise doesn't happen)" note in the description, I'm wondering if we want to do one linter per MR rather than all of them at once.
    2. I'm not sure about the ImplicitDiv linter. If people want to be explicit and do %div.some-class instead of .some-class, I'm fine with it.
  • Kushal Pandya
    Kushal Pandya @kushalpandya ·
    Author Developer

    @rspeicher I'm all in for idea of having MR per Linter 💯 and while this MR has a ton of changes, over 90% changes are purely cosmetic, especially the biggest offender SpaceInsideHashAttributes has over 900 modifications across the files is just addition of spaces which have absolutely no effect on behavior.

    What needs to be reviewed thoroughly is AltText, HtmlAttributes and SpaceBeforeScript, all of which have significantly low number of changes (which are not cosmetic). Also, I've kept commits for fixing each linter separate so that it is easier for reviewer to distinguish between changes. 🙂

  • Alfredo Sumaran
    Alfredo Sumaran @asumaran ·
    Contributor

    @kushalpandya are you going to create a MR for each rule?

  • Kushal Pandya
    Kushal Pandya @kushalpandya ·
    Author Developer

    @alfredo1 if we conclude to do so, otherwise I still feel it shouldn't be a problem to distinguish changes for each linter in this MR itself and push all the changes in one go without further delays. 🙂

  • Alfredo Sumaran
    Alfredo Sumaran @asumaran ·
    Contributor

    Alright, @kushalpandya please do a last rebase so I can set it to MWPS 👍

  • Alfredo Sumaran assigned to @kushalpandya

    assigned to @kushalpandya

  • Kushal Pandya added 232 commits

    added 232 commits

    • 3802abf7...44fddd29 - 215 commits from branch master
    • 21777aa1 - HAMLLint: Enable AltText rule
    • 71dc50f0 - HAMLLint: fix AltText offences
    • ef3744d0 - HAMLLint: Enable ClassAttributeWithStaticValue rule
    • 7b1944f9 - HAMLLint: Fix ClassAttributeWithStaticValue offences
    • 08e083db - HAMLLint: Enable FinalNewline rule
    • 1974deef - HAMLLint: Fix FinalNewline offences
    • fa432f0c - HAMLLint: Enable HtmlAttributes rule
    • 71000b24 - HAMLLint: Fix HtmlAttributes offences
    • 7157f585 - HAMLLint: Enable ImplicitDiv rule
    • dd5ffd9c - HAMLLint: Fix ImplicitDiv offences
    • 6f6f546b - HAMLLint: Enable SpaceBeforeScript rule
    • 598d8cab - HAMLLint: Fix SpaceBeforeScript offences
    • df4f896b - HAMLLint: Enable TrailingWhitespace rule
    • 97f3c8f3 - HAMLLint: Fix TrailingWhitespace offences
    • 8e2a76d2 - HAMLLint: Enable SpaceInsideHashAttributes rule
    • fb3e3654 - HAMLLint: Fix SpaceInsideHashAttributes offences
    • e0f765ae - HAMLLint: Fix SpaceInsideHashAttributes offence

    Compare with previous version

    Toggle commit list
  • Kushal Pandya
    Kushal Pandya @kushalpandya ·
    Author Developer

    Hi @alfredo1 I've resolved conflicts with latest rebase, please check.

  • Kushal Pandya assigned to @alfredo1

    assigned to @alfredo1

  • Kushal Pandya added 2 commits

    added 2 commits

    • a54c9b7f - HAMLLint: Fix TrailingWhitespace offense post-rebase
    • f79680c5 - HAMLLint: Fix ImplicitDiv offence post-rebase

    Compare with previous version

  • Alfredo Sumaran
    Alfredo Sumaran @asumaran ·
    Contributor

    Thanks @kushalpandya works perfect! 👍

  • Alfredo Sumaran merged

    merged

  • Alfredo Sumaran mentioned in commit 8dc2163c

    mentioned in commit 8dc2163c

  • Kushal Pandya mentioned in issue #22072 (closed)

    mentioned in issue #22072 (closed)

  • James Lopez
    James Lopez @jameslopez ·
    Maintainer

    @alfredo1 @kushalpandya was there a relevant EE counterpart for this? I had a bunch of errors merging CE => EE https://gitlab.com/gitlab-org/gitlab-ee/builds/8391569 - I will try to fix this myself for now...

  • Kushal Pandya
    Kushal Pandya @kushalpandya ·
    Author Developer

    Hi @jameslopez, sorry I was travelling. Are the errors resolved or you're still facing it?

  • James Lopez
    James Lopez @jameslopez ·
    Maintainer

    @kushalpandya no worries! - all solved now!

  • 🤖 GitLab Bot 🤖 added groupcode review label

    added groupcode review label

    • Please register or sign in to reply