Part of #1266442: [meta] Implement automated code style checks for core.

Once core is cleaned up, qa.drupal.org should take code style into account when determining pass or fail for css coding standards

See #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core for current progress, once that is done it can be added to the testbot

CommentFileSizeAuthor
#4 csslint.xml_.txt196.58 KBattiks
#4 i2575497-4.patch4.28 KBattiks
#3 i2575497-3.patch4.27 KBattiks
#2 i2575497-2.patch4.25 KBattiks

Comments

attiks created an issue. See original summary.

attiks’s picture

attiks commented
Status: Active » Needs review
StatusFileSize
new i2575497-2.patch4.25 KB

Checks css files using the csslintrc file already present in core.

attiks’s picture

attiks commented
StatusFileSize
new i2575497-3.patch4.27 KB

New patch, creating the artifacts dir

attiks’s picture

attiks commented
StatusFileSize
new i2575497-4.patch4.28 KB
new csslint.xml_.txt196.58 KB

New patch excluding core/tests since it has css files containing UTF16 chars

elachlan’s picture

elachlan commented

Would it make sense to add csslint to the container definition? Similar to the changes for #2600626: [PP-1] Ensure availability of node/npm in the testrunners?

elachlan’s picture

elachlan commented
Related issues: +#1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core, +#2605308: Add csslint to the testrunners

Created a new issue to add csslint to the container #2605308: Add csslint to the testrunners

elachlan’s picture

elachlan commented

Attiks could you please review the patch in the child issue #2605308: Add csslint to the testrunners?

elachlan’s picture

elachlan commented
Priority: Normal » Major
Parent issue: #1266444: DrupalCI should return failed for poor code style - php » #1299710: [meta] Automate the coding-standards part of patch review
Related issues: +#1266444: DrupalCI should return failed for poor code style - php
lewisnyman’s picture

lewisnyman
he/him
commented

Maybe this test could be tweaked so new patches don't add any more errors?

elachlan’s picture

elachlan commented

It is a separate job. So we can have it pass until all errors are removed and then we switch it to fail.

Here is the output from #1299710: [meta] Automate the coding-standards part of patch review
test results

catch’s picture

catch
he/him
Primary language English
commented

Yes warn rather than fail, so we can have everything switched on and not blocked on core would be great.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Just something I noticed, though I'm sure you're all aware:

+++ b/src/DrupalCI/Plugin/JobTypes/csslint/CSSLintJob.php
@@ -0,0 +1,67 @@
+    'DCI_RunScript' => "csslint --exclude-list=core/vendor,core/assets/vendor/,core/tests --format=junit-xml core > artifacts/csslint.xml",

core/vendor is now vendor

yesct’s picture

yesct commented
Issue tags: +Coding standards
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work

Setting to 'needs work' based on #12.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Addressing blockers for this issue: #2654650: [meta] Jobs for linting and coding standards

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Component: Miscellaneous » Jobs and Job Handling
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Assigned: Unassigned » Mixologic

Im working on this this week.

elachlan’s picture

elachlan commented

@Mixologic - Thanks for the update!

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs work » Postponed

Okay, so it turns out the the .csslintrc file that comes with core, which is what we want to use as a fallback if there isnt anything defined for a project, is broken.

This is blocked on fixing core: #2799603: .csslintrc file format is invalid and results in "important is not a valid option"

elachlan’s picture

elachlan commented
Related issues: +#2799603: .csslintrc file format is invalid and results in "important is not a valid option"
Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Title: DrupalCI should return failed for poor code style - CSS » DrupalCI should report code style issues - CSS

  • Mixologic committed e837976 on 2575497-csslinting-support 
    Issue #2575497: adds csslint plugin
hestenet’s picture

hestenet
He/Him
Location Portland, OR 🇺🇸
commented
Status: Postponed » Needs work

The core issue is now fixed - un-postponing this one.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented
Status: Needs work » Fixed

This has been deployed to production. We have csslint checkstyle results now.

Mixologic’s picture

Mixologic
He/Him/His
Primary language English
Location Portland, OR
commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.