Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
If we want to have csslint turned on for core we need to exclude all the files that currently fail. That way we can lint the passing files and not introduce regression. Then as files are fixed we can ensure that no regressions are made to that file.
Basically #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core unfortunately didn't manage to fix all the css.
Proposed resolution
Add all currently failing files to the exclude list.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#21 | fix_csslint_by-2864753-21.patch | 11.32 KB | Mixologic |
#15 | 2864753-15.patch | 12.5 KB | alexpott |
#15 | 9-15interidff.txt | 328 bytes | alexpott |
#9 | 2864753-9.patch | 12.47 KB | alexpott |
#9 | 7-9-interidff.txt | 666 bytes | alexpott |
Comments
Comment #2
alexpottOutput of csslint ./core with this:
Comment #3
xjmCan we insert some linebreaks?
Comment #4
alexpottI tried that but it just didn't work - unfortunately. I tried it with indentation and without.
Comment #5
xjmBut HEAD has a linebreak somehow?
Comment #6
alexpottThe linebreak is not effective and we never scan vendor anyway only things in ./core. :(
Comment #7
alexpottReading the csslint made me aware that they support json in the .rc file - yay! So we have a sensible solution.
Same output as before just a more sensible and easier to read file.
Comment #8
alexpottThere still one css fail with #2 apparently... https://dispatcher.drupalci.org/job/drupal_patches/8478/
Comment #9
alexpottFixed the thing by ignoring the file. Which in this case is correct because this css file only exists for to test css aggregation.
Comment #10
xjm+1, that's way more readable.
Comment #11
Mile23I can verify that the patch removes the error messages from csslint locally.
Also it leaves a bunch of errors in 8.4.x, so maybe commit here and then move this issue to 8.4.x.
The trick then is what's the process for remembering that you've excluded files from csslint? So this needs a follow-up where people fix those files.
Comment #12
alexpott@Mile23 - where are the warning on on 8.4.x? https://dispatcher.drupalci.org/job/drupal_patches/8526/ looks fine to me.
Comment #13
Mile23Maybe it was something unique to my local... Can't reproduce it now.
Comment #14
lauriiiShould we also consider excluding core/node_modules?
Comment #15
alexpottNo harm in adding that location. Whilst it doesn't exist now and never will be checked in the reason for adding it to .gitignore is the same here. Whilst not in scope to fix the actual issue at hand there is no harm at all in adding it now.
removing needs followup tag because this was introduced in #11 and that's been resolved. #1190252: [573] Use csslint as a weapon to beat the crappy CSS out of Drupal core is the real followup to this issue.
Comment #16
Mile23For core, the testbot is saying
csslint --format=checkstyle-xml --config=.csslint .
Comment #2 says
csslint ./core
Should the testbot only lint core/?
Aside: The drupalci csslint task doesn't seem to leave a lot of evidence of what it did in either the console or the artifacts.
Comment #17
alexpott@Mile23 yep we should definitely only lint ./core - no other css should be linted as part of a core run.
Comment #18
Mile23Voila: #2865234: CSSLint should only lint core/
Comment #19
Mixologic.csslintrc does take a json format, but I think it would be preferable to stick with the original text based format because I *think* that it will allow for comments where I know json does not. For readability, we can just continue the pattern thats there now, namely with each exclude dir on its own line followed by a comma.
A comment as to why we're excluding these files would also be nice (provided that Im correct in thinking we can actually do that). Otherwise LGTM.
Comment #20
alexpott@Mixologic unfortunately that means the entire list of files needs to be on one line. See earlier comments on this issue.
Comment #21
MixologicIm not sure what issues you were having in #6 but linebreak does seem to work for me.
I put a comment in just to see how it would work, and, well, they're a little hacky. If you add a comma after the comment it works. otherwise it confuses the next exclude file. So they arent really comments per se, but just slip through because they dont match anything. So It doesnt really seem that "ability to comment" is a feature of the plaintext config file.
I would still prefer to use the non-json version as we're going to want contrib to use their own csslint files, and a hand edited json file can be harder to maintain, and We'll end up getting questions on how to exclude files from testing, so there is a little bit of support burden avoidance baked into my desires.
This is the output I get with this patch:
Comment #22
alexpottWell json validates and plain-text doesn't :) But this looks great to me. Less change as well. Testing locally and can confirm it works for me.
I think the comment whilst weird is okay and more helpful than not having one.
Comment #23
alexpottNote that we've opened #2866840: Use stylelint as opposed to csslint in DrupalCI and are thinking about moving to stylelint. See #2865971: Use stylelint as opposed to csslint in core
Comment #24
joelpittetLet's wait till #2865971: Use stylelint as opposed to csslint in core is resolved before making more work for ourselves
Comment #25
alexpott@joelpittet it would great to have your +1 on #2865971: Use stylelint as opposed to csslint in core
Comment #28
joelpittetClosing, running with stylelint