Skip to content

[4.0] Removing Hound and moving to Drone#30196

Merged
rdeutz merged 11 commits intojoomla:4.0-devfrom
Hackwar:j4stylelint
Jul 29, 2020
Merged

[4.0] Removing Hound and moving to Drone#30196
rdeutz merged 11 commits intojoomla:4.0-devfrom
Hackwar:j4stylelint

Conversation

@Hackwar
Copy link
Copy Markdown
Member

@Hackwar Hackwar commented Jul 26, 2020 •
edited
Loading

For Javascript and SCSS codestyle, we are using Hound to check against our codestyle standards. Due to unknown reasons, Hound failed to catch several violations to our standards, since it only reports on files that have been modified instead of the whole codebase each time. So we would like to have a full check on all our SCSS and JS files each time.

Coincidentally we already do JS codestyle checks on each build/PR in drone, so we can simply drop the whole JS codestyle stuff from Hound.

Regarding SCSS a solution might be to simply call scss-lint in Drone and be done with it, but that brings up another issue: scss-lint is Ruby-based and is not really maintained anymore. SCSS at one point was Ruby-based, but has since been rewritten in NodeJS and thus a Ruby-based linter adds yet another environment for the developer to add just for this. Long story short: scss-lint is not the prefered tool here anymore.

The solution seems to be stylelint, which can be simply installed during setup of the project with npm i. However, now we have to adopt the configuration from scss-lint to stylelint. The following abstract of the configuration file is still missing in the stylelint configuration. Maybe we drop some of these, maybe we enforce others. That would be up for discussion here.

linters:
  BorderZero:
    enabled: true
    convention: zero # or `none`
    exclude:
      - _normalize.scss

  Comment:
    enabled: true
    exclude:
      - _normalize.scss
      - bootstrap.scss
    style: silent

  ElsePlacement:
    enabled: true
    style: same_line # or 'new_line'

  IdSelector:
    enabled: true

  ImportPath:
    enabled: true
    leading_underscore: false
    filename_extension: false

  NameFormat:
    enabled: true
    allow_leading_underscore: true
    convention: hyphenated_lowercase # or 'camel_case', or 'snake_case', or a regex pattern

  PseudoElement:
    enabled: true

  SpaceAroundOperator:
    enabled: true
    style: one_space # or 'at_least_one_space', or 'no_space'

  UrlFormat:
    enabled: true

I would be very happy if someone could support me in fixing the codestyle violations and to finish the stylelint configuration.

@joomla-cms-bot joomla-cms-bot added the NPM Resource Changed This Pull Request can't be tested by Patchtester label Jul 26, 2020
@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Jul 28, 2020

@C-Lodder since you initially setup the scss-lint configuration, can you give some feedback here? Maybe also help with the violations which came up?

@C-Lodder
Copy link
Copy Markdown
Member

C-Lodder commented Jul 29, 2020 •
edited
Loading

Can you re-run Drone please. It's showing results for a previous commit

@C-Lodder
Copy link
Copy Markdown
Member

C-Lodder commented Jul 29, 2020

Personally I would extend stylelint's standard config rather than using your own rules. Meaning that if any new standards come into play in the future, you don't need to manually add them, but instead simply update the dependency.
You can then built on top of it.

Just run a test locally with stylelint. The only errors being returned now are:

  • max-nesting-depth
  • selector-max-compound-selectors

I don't want to really dive into Atum's SCSS to be honest.

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Jul 29, 2020

Thank you for your feedback. Right now I just wanted to move the infrastructure over, I'm open to changing rules afterwards. Since you seem to have been the one to introduce this, I was hoping that you could elaborate a bit on why you chose these rules and if those left are necessary from your point of view or if we could drop them. It sounds a bit like you took a default style and maybe modified it a bit. Unless you have any objections, I would simply run with what is in this PR now, fix the last errors and then be done with it...

@C-Lodder
Copy link
Copy Markdown
Member

C-Lodder commented Jul 29, 2020

I originally copied over the rules used by Bootstrap when they were using the Ruby scss-lint gem. Can't quite remember if I made any tweaks as it was a few years ago now, but you can change the rules to whatever you think is best for Joomla ;)

@Hackwar
Copy link
Copy Markdown
Member Author

Hackwar commented Jul 29, 2020

Thank you. I will then keep it as it is and later migrate to another, established codestyle... 🙈

@Hackwar Hackwar marked this pull request as ready for review July 29, 2020 10:09
@Hackwar Hackwar requested review from rdeutz and wilsonge as code owners July 29, 2020 10:09
@Hackwar Hackwar mentioned this pull request Jul 29, 2020
Closed
@C-Lodder
Copy link
Copy Markdown
Member

C-Lodder commented Jul 29, 2020

You can also remove Gemfile and Gemfile.lock from the root

@rdeutz rdeutz merged commit 4908ca0 into joomla:4.0-dev Jul 29, 2020
@Hackwar Hackwar mentioned this pull request Jul 29, 2020
Merged
@Hackwar Hackwar deleted the j4stylelint branch July 29, 2020 11:35
@zero-24 zero-24 added this to the Joomla 4.0 milestone Jul 30, 2020
@brianteeman brianteeman mentioned this pull request Aug 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rdeutz rdeutz Awaiting requested review from rdeutz

@wilsonge wilsonge Awaiting requested review from wilsonge

Assignees

No one assigned

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester Unit/System Tests

Projects

None yet

Milestone

Joomla 4.0

Development

Successfully merging this pull request may close these issues.

5 participants

@Hackwar @C-Lodder @rdeutz @zero-24 @joomla-cms-bot