981610 - Activate the CSS Linter as a pre-commit hook

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Would be nice to have it as a pre-commit hook. A system like the one which is used for jshint files that are failing would be nice too to turn it on. It would be better if this system can also have the number of errors/warnings in the file, and so people won't be able to add new one, but only to remove some.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

At some points the linter should be activated as a pre-commit hook. In order to do so we will likely have to have a file to exclude some css files, but I hope we can reduce the list to a minimum.

So let's starts to land some css clean up into some bugs before finishing the pre-commit hook patch...

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bluetooth.warnings.patch

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: browser.warnings.patch

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: calendar.warnings.patch

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: camera.warnings.patch

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: clock.warnings.patch

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: costcontrol.warnings.patch

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: email.warnings.patch

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: fl.warnings.patch

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: gallery.warnings.patch

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: homescreen.warnings.patch

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: keyboard.warnings.patch

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: pdfjs.warnings.patch

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: settings.warnings.patch

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: sms.warnings.patch

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: system.warnings.patch

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: video.warnings.css

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Those patches are just some small and safe (in theory) removals of some css warnings/errors.

There is still errors and warnings with those fixes, but a bit less. Asking reviews to you on some of those patches sounds also a simple and friendly way to make you know about the css linter :)

The idea is to be able to turn on the linter with as few ignored files as possible. The rules for the linter can be discussed as much as you want (for the moment i enabled only things that seems to make sense to me, but i have no strong opinions), but those fixes should be pretty straightforward imho.

If you want to run the linter yourself on the app, just do |APP=... make csslint| until the pre-commit hook lands.

Comment 19

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8390498 [details] [diff] [review]
homescreen.warnings.patch

Sorry Vivien, attach the patch, I think that it is just a log :)

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Cristian Rodriguez (:crdlc) from comment #19)
> Comment on attachment 8390498 [details] [diff] [review]
> homescreen.warnings.patch
> 
> Sorry Vivien, attach the patch, I think that it is just a log :)

oups :) I will attached the real patch!

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: contacts.warnings.patch

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: dialer.warnings.patch

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: ftu.warnings.patch

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: homescreen.warnings.patch

Comment 25

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8390566 [details] [diff] [review]
homescreen.warnings.patch

All changes are done in the ev.me code so I prefer that Ran will take a look to this better than me. Thx!

Comment 26

[Jose Manuel Cantera]

Comment on attachment 8390562 [details] [diff] [review]
contacts.warnings.patch

Review of attachment 8390562 [details] [diff] [review]:
-----------------------------------------------------------------

thanks

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: precommit.css.patch

Here is the pre-commit hook.

For the moment the list of exclusion is contains in build/csslint/xfail.list.
The file is still empty as I intend to populate it before landing as it does not really makes sense to do it right now. I also relaxed a bit the warnings part since have having played with the linter a bit I have some doubts that some of the rules are going to be fixed one day.

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8390563 [details] [diff] [review]
dialer.warnings.patch

Review of attachment 8390563 [details] [diff] [review]:
-----------------------------------------------------------------

impressive bug :)

Comment 29

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8390564 [details] [diff] [review]
ftu.warnings.patch

Thanks Vivien!

Comment 30

[David Flanagan [:djf]]

Comment on attachment 8390496 [details] [diff] [review]
fl.warnings.patch

Review of attachment 8390496 [details] [diff] [review]:
-----------------------------------------------------------------

just removes an unused class. trivially correct.

Comment 31

[David Flanagan [:djf]]

Comment on attachment 8390497 [details] [diff] [review]
gallery.warnings.patch

trivially correct patch that just removes unneeded units from 0 values.

Comment 32

[David Flanagan [:djf]]

Comment on attachment 8390505 [details]
video.warnings.css

trivially correct patch that just removes units from zero values.

Comment 33

[Brendan Dahl [:bdahl]]

Comment on attachment 8390500 [details] [diff] [review]
pdfjs.warnings.patch

Review of attachment 8390500 [details] [diff] [review]:
-----------------------------------------------------------------

Upstreamed https://github.com/mozilla/pdf.js/pull/4454

Comment 34

[Evelyn Hung]

Comment on attachment 8390501 [details] [diff] [review]
settings.warnings.patch

thanks!

Comment 35

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8390566 [details] [diff] [review]
homescreen.warnings.patch

Review of attachment 8390566 [details] [diff] [review]:
-----------------------------------------------------------------

Good catches! :)

Comment 36

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just merged the css changes where I already got a r+ since all of them are atomic.

https://github.com/mozilla-b2g/gaia/commit/da62d798a8a7b9e4d24c40265e171756731d856a

Comment 37

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8390499 [details] [diff] [review]
keyboard.warnings.patch

+  margin: -0.6rem 0 0rem 0; /* I don't know why we need the top margin */

0rem -> should be 0

Comment 38

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Jan Jongboom [:janjongboom] from comment #37)
> Comment on attachment 8390499 [details] [diff] [review]
> keyboard.warnings.patch
> 
> +  margin: -0.6rem 0 0rem 0; /* I don't know why we need the top margin */
> 
> 0rem -> should be 0

Good catch thanks.

Comment 39

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug981610.keyboards.patch

Comment 40

[Alexandre Poirot [:ochameau]]

Comment on attachment 8390954 [details] [diff] [review]
precommit.css.patch

Review of attachment 8390954 [details] [diff] [review]:
-----------------------------------------------------------------

Doesn't apply/work. Isn't there a missing patch?

::: tools/pre-commit
@@ +94,5 @@
> +
> +$XULRUNNER_SDK $XPCSHELL_SDK \
> +  -e "const GAIA_BUILD_DIR='file://$(pwd)/build/'" \
> +  -f build/xpcshell-commonjs.js \
> +  -e "quit(require('csslint').lint('$(pwd)', '$changed_files'));"

What about calling a Makefile's target instead (like what we are doing with gjslint)?
It looks weird to pull Makefile variables with such special echo rule.

@@ +108,1 @@
>  for file in $(git diff --staged --name-only); do

pre-commit doesn't apply, nor works as $changed_files only track .js files...

Also, even if I fixed that, I get some stange behavior:

$ git diff
diff --git a/apps/browser/style/settings.css b/apps/browser/style/settings.css
index 186fba6..db2d535 100644
--- a/apps/browser/style/settings.css
+++ b/apps/browser/style/settings.css
@@ -1,3 +1,4 @@
+
 #settings ul {
   margin: 0;
   padding: 0;
alex@ubuntu:~/gaia$ git commit apps/browser/style/settings.css 
apps/browser/style/settings.css
There were errors while linting the files, please see above.

We don't see any meaningfull error message.

Comment 41

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug981610.precommit

Not sure why you didn't see anything in the console.

I forgot to fix the changed_files stuff since I initially develop the pre-commit hook into a shell script.

I killed the build/xulrunner_config file but keep the command to call the linter in the pre-commit file as it seems that the Makefile is already huge and I prefer to keep the logic where it belongs.

I will walk and seat next to you to see if there is anything wrong and why nothing is showned on your console (if it still happens).

Comment 42

[Alexandre Poirot [:ochameau]]

Comment on attachment 8392880 [details] [diff] [review]
bug981610.precommit

Review of attachment 8392880 [details] [diff] [review]:
-----------------------------------------------------------------

Works great now!
That would be really great if you can move the xpcshell call in the Makefile so that we get a common place where we call it.

Comment 43

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug981610.shared.patch

Comment 44

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Just merged the linter: https://github.com/mozilla-b2g/gaia/commit/c250da9f8fdc511ad718ba594a0aa60a5959e74b

It makes me a bit stressed since if it's buggy it may be painful for people. If there is any issue, please don't hesitate to backout the changeset c250da9f8fdc511ad718ba594a0aa60a5959e74b and I will fix the issue before relanding.

Comment 45

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8393008 [details] [diff] [review]
bug981610.shared.patch

Thanks Vivien, that will help us catching some errors!

Comment 46

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Keyboard patch: https://github.com/mozilla-b2g/gaia/commit/5a0cb2aad7fed7217638fe6248ed131bf0665ad5

Comment 47

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

shared patch: https://github.com/mozilla-b2g/gaia/commit/71df836011d1d116c94551c23ffce522ded958ae

I also realized that there was an issue with the shared/ folder beeing not taken correctly by the pre-commit hook, so I fixed that and land it as well.

Comment 48

[Salvador de la Puente González [:salva]]

Comment on attachment 8390494 [details] [diff] [review]
costcontrol.warnings.patch

Review of attachment 8390494 [details] [diff] [review]:
-----------------------------------------------------------------

The patch does not apply on master. Maybe something changed. Here in:

--- a/apps/costcontrol/style/views/firsttime.css	
+++ a/apps/costcontrol/style/views/firsttime.css	
@@ -1,1 +1,0 @@ 
-    color: #fff;

color: #fff does not exist. But I found a dupllicated entry with color: #000 instead.

Comment 49

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Grrr. Someone backouted some of the patches with a bad merge backout..... Relanded: https://github.com/mozilla-b2g/gaia/commit/3478dc21f7a8a836a51ee0ee0f1ed1fed1a8255d

Comment 50

[Anthony Ricaud (:rik)]

This is done, let's close.