[qa] Added linting of JS and CSS #173 #202

niteshsinha17 · 2021-05-27T16:39:36Z

It also formats css files when we run run-qa-format
closes #173

atb00ker

Please test my recommendations in success case and fail case.
This openwisp-qa-check will never fail our CI; hence marking as request changes! 😄

atb00ker · 2021-05-31T10:09:36Z

openwisp-qa-check

@@ -60,6 +60,16 @@ runcheckendline() {
  fi
 }

+runstylelintcheck() {
+  echo "RUNNING: Stylelint checks"
+  stylelint -f verbose $(find . -type f -name "*.css")


I think this command would spit a lot of unnecessary information even when the test passes.
To avoid it, checkout the how the other functions are suppressing output on success.
Example:

runstylelintcheck() { stylelint $(find . -type f -name "*.css") && echo "SUCCESS: Stylelint check successful!" || { echoerr "ERROR: Stylelint check failed! Hint: did you forget running openwisp-qa-format?" FAILURE=1 } }

atb00ker · 2021-05-31T10:12:54Z

openwisp-qa-check

+
+runjshintcheck() {
+  echo "RUNNING: jshint checks"
+  jshint --verbose $(find . -type f -name "*.js" -a ! -path "*vendor/*.js")


Just like above it will give a lot of output even when the test passes.

runjshintcheck() { jshint $(find . -type f -name "*.js" -a ! -path "*vendor/*.js") && echo "SUCCESS: Stylelint check successful!" || { echoerr "ERROR: Stylelint check failed! Hint: did you forget running openwisp-qa-format?" FAILURE=1 } }

niteshsinha17 · 2021-05-31T13:07:42Z

@atb00ker I have one doubt. I am writing these test at two placed openwisp-qa-checks and run-qa-checks. If I do not write it in run-qa-checks it never runs on run-qa-checks command but other tests run properly. Am I missing something?

atb00ker · 2021-05-31T14:42:29Z

You need to do pip install .[qa,rest] to update the script.

nemesifier

This commit shouldn't be here: 2675cff

nemesifier · 2021-06-01T16:25:42Z

openwisp-qa-check

+    SKIP_STYLELINT=true
+    ;;
+  --skip-jshint)
+    SKIP_JSHINT=true


Great to see that we are adding this in this script! Will be great for reusability.

But we definitely should not do it this way @niteshsinha17 @atb00ker, because it will cause some existing builds to fail because these linters may not be installed.

We must introduce this feature in a backward compatible way, so either by default these linters should not run (the options could be something like --css and --js to activate the respective linters) or before running the shell script should verify the availability of the linter program and skip if it's not present.

Moreover, we'll have to update the show_help function above in this file and the README of the module to mention these options.

You are right. I was also thinking about this.

closes #173

niteshsinha17 · 2021-06-02T08:14:55Z

run-qa-checks

@@ -2,7 +2,10 @@
 set -e

 qa-checks () {
-    openwisp-qa-check --migration-path "./tests/test_project/migrations"
+    openwisp-qa-check \


@nemesisdesign @atb00ker Its ready for review. We will need to add css and js test for each module like this to run them in CI.

We can document about this addition here. http://openwisp.io/docs/developer/contributing.html#coding-style-conventions

This section of the README needs to be updated: https://github.com/openwisp/openwisp-utils#openwisp-qa-check

atb00ker

Tested, LGTM. 😄

I just realized.
The optoins --stylelint & --jshint describe much better what's going to run.
I wouldn't be able to guess what --css and --js are going to do.

atb00ker · 2021-06-02T14:58:31Z

openwisp-qa-check

+      echoerr "ERROR: Stylelint check failed! Hint: did you forget running openwisp-qa-format?"
+      FAILURE=1
+    }
+


Unintended space! 😄

is this resolved?

nemesifier

Still not ready, see below.

I just realized.
The optoins --stylelint & --jshint describe much better what's going to run.
I wouldn't be able to guess what --css and --js are going to do.

@niteshsinha17 what do you think about this suggestion? I'm fine with both nomenclatures.
The advantage of using css and js is that in the future we change the underlying linting library, we don't need to change the flag name.
Maybe we can use csslinter and jslinter to make it extra clear?

nemesifier · 2021-06-02T15:44:16Z

run-qa-checks

@@ -2,7 +2,10 @@
 set -e

 qa-checks () {
-    openwisp-qa-check --migration-path "./tests/test_project/migrations"
+    openwisp-qa-check \


This section of the README needs to be updated: https://github.com/openwisp/openwisp-utils#openwisp-qa-check

nemesifier · 2021-06-02T15:45:19Z

openwisp-qa-check

@@ -60,6 +68,26 @@ runcheckendline() {
  fi
 }

+runstylelintcheck() {
+  stylelint $(find . -type f -name "*.css") &&


what happens if stylelint is not installed? Can you make sure the function skips with a warning?
We need this to avoid breaking existing builds which are using the master version of this module.

Added a check for this

nemesifier · 2021-06-02T15:45:29Z

openwisp-qa-check

+      echoerr "ERROR: Stylelint check failed! Hint: did you forget running openwisp-qa-format?"
+      FAILURE=1
+    }
+


is this resolved?

nemesifier · 2021-06-02T15:45:36Z

openwisp-qa-check

+}
+
+runjshintcheck() {
+  jshint --verbose $(find . -type f -name "*.js" -a ! -path "*vendor/*.js") &&


what happens if jshint is not installed? Can you make sure the function skips with a warning?
We need this to avoid breaking existing builds which are using the master version of this module.

Added a check for this

atb00ker

I think it's close to merge! 😄
Two comments:

atb00ker · 2021-06-04T14:46:48Z

README.rst

 Shell script to automatically format Python code. It runs ``isort`` and ``black``.
+It also runs ``stylelint`` to format CSS code.


Shell script to automatically format Python code. It runs isort. black and stylelint.

I think its better.
Shell script to automatically format Python and CSS code. It runs isort. black and stylelint.

Please mention that the command won't fail if stylelint is not installed but will skip instead.

atb00ker · 2021-06-04T14:49:17Z

openwisp-qa-check

+  package='stylelint'
+  if which node > /dev/null
+    then
+      if [ `npm list -g | grep -c $package` -eq 0 ]; then


What if I have npm? Does npm come bundled with node?
What do you think about using whereis stylelint instead?
whereis comes out of the box and works great in such situations!

I will check this and let you know

@atb00ker How should I use whereis for this situation?

@niteshsinha17 What do you mean? What's the challenge? Can you please elaborate?
P.S: You can also use which stylelint & which jshint.

Yes which stylelint will work fine.
I am not getting how to write it with whereis I tried multiple ways but it was not working as expected. Can you please share exact syntax with whereis

Some version of whereis stylelint | grep /stylelint should work but don't worry about it, let's focus on which only, whereis is not a requirement. 😄

nemesifier

The changes to the CSS are working fine, only one detail remaining to handle, see below.

nemesifier · 2021-06-06T23:02:14Z

openwisp-qa-format

@@ -3,3 +3,4 @@ set -e

 isort .
 black -S .
+stylelint $(find . -type f -name "*.css") --fix


please ensure stylelint is available before trying to run it

coveralls · 2021-06-07T07:09:15Z

Coverage remained the same at 99.295% when pulling eb489f1 on issues/173-add-linting-of-js-and-css into f32eb09 on master.

niteshsinha17 · 2021-06-07T07:20:14Z

@nemesisdesign and @atb00ker Updated this PR once again.
Changes made

This time added some more rules in stylelint
I am failing checks when stylelint and jshint are not installed.
Made changes requested by you last time.

Its ready now

atb00ker

LGTM! 😄

nemesifier · 2021-06-07T23:00:54Z

openwisp-qa-format

+    stylelint $(find . -type f -name "*.css") --fix
+  else
+    echo "MODULE NOT FOUND: Please install stylelint"
+    exit 1


once more time, I think this is wrong. Didn't we agree this feature must be backward compatible?

So why fail if styleling is not installed? This command must not fail if stylelint is missing. It could simply say "The program "stylelint" is not installed, Skipping CSS auto-formatting." or something similar.

nemesifier · 2021-06-07T23:04:09Z

README.rst

 Shell script to automatically format Python code. It runs ``isort`` and ``black``.
+It also runs ``stylelint`` to format CSS code.


Please mention that the command won't fail if stylelint is not installed but will skip instead.

niteshsinha17 · 2021-06-08T12:38:19Z

@nemesisdesign I have fixed them. I will be more careful next time.

niteshsinha17 · 2021-06-08T17:30:25Z

openwisp-qa-check

+    stylelint verbose $(find . -type f -name "*.css") &&
+      echo "SUCCESS: Stylelint check successful!" ||
+      {
+        echoerr "ERROR: Stylelint check failed! Hint: did you forget running openwisp-qa-format? You may need some manual fixing."


I think we can change it. to "ERROR: Stylelint check failed! Hint: did you forget running openwisp-qa-format? You may need to do some manual fixing."

niteshsinha17 self-assigned this May 27, 2021

niteshsinha17 added this to In progress in [GSoC 2021] Modern UI/UX via automation May 27, 2021

niteshsinha17 moved this from In progress to Needs review in [GSoC 2021] Modern UI/UX May 27, 2021

atb00ker requested changes May 31, 2021

View reviewed changes

atb00ker moved this from Needs review to In progress in [GSoC 2021] Modern UI/UX May 31, 2021

nemesifier requested changes Jun 1, 2021

View reviewed changes

niteshsinha17 added 3 commits June 2, 2021 12:30

[qa] Added linting of JS and CSS #173

3439473

closes #173

[chores] Added missing blank line

6f4aed0

[change] Removed css and js skip

5a05b8c

niteshsinha17 force-pushed the issues/173-add-linting-of-js-and-css branch 2 times, most recently from a10b212 to 24b028d Compare June 2, 2021 07:46

niteshsinha17 commented Jun 2, 2021

View reviewed changes

niteshsinha17 moved this from In progress to Needs review in [GSoC 2021] Modern UI/UX Jun 2, 2021

atb00ker reviewed Jun 2, 2021

View reviewed changes

nemesifier requested changes Jun 2, 2021

View reviewed changes

atb00ker moved this from Needs review to In progress in [GSoC 2021] Modern UI/UX Jun 2, 2021

niteshsinha17 moved this from In progress to Needs review in [GSoC 2021] Modern UI/UX Jun 3, 2021

atb00ker approved these changes Jun 4, 2021

View reviewed changes

atb00ker moved this from Needs review to In progress in [GSoC 2021] Modern UI/UX Jun 4, 2021

niteshsinha17 added 3 commits June 4, 2021 22:00

[fix] Fixed typo in opwisp-qa-check

4e5700d

[chores] Updated docs and added checks for node modules

2053e42

[chores] Tested ci when no node modules are installed

a4f9322

niteshsinha17 force-pushed the issues/173-add-linting-of-js-and-css branch from 836f8e8 to a4f9322 Compare June 4, 2021 16:31

nemesifier changed the base branch from dev to master June 6, 2021 23:03

nemesifier requested changes Jun 6, 2021

View reviewed changes

niteshsinha17 added 2 commits June 7, 2021 12:36

[chores] Minnor changes

f44f3c2

[chores] Minnor changes

b2ea06d

[chores] Do not allow to push when stylint and jshint not installed

bd62a02

niteshsinha17 moved this from In progress to Needs review in [GSoC 2021] Modern UI/UX Jun 7, 2021

atb00ker approved these changes Jun 7, 2021

View reviewed changes

nemesifier requested changes Jun 7, 2021

View reviewed changes

[chores] Improvement in messages

95833ae

[chores] Minor improvements

005b4d1

nemesifier approved these changes Jun 8, 2021

View reviewed changes

niteshsinha17 commented Jun 8, 2021

View reviewed changes

[chores] Further minor improvements

eb489f1

nemesifier approved these changes Jun 8, 2021

View reviewed changes

nemesifier merged commit 1ae1682 into master Jun 8, 2021

[GSoC 2021] Modern UI/UX automation moved this from Needs review to Done Jun 8, 2021

nemesifier deleted the issues/173-add-linting-of-js-and-css branch June 8, 2021 17:35

		Shell script to automatically format Python code. It runs ``isort`` and ``black``.
		It also runs ``stylelint`` to format CSS code.

[qa] Added linting of JS and CSS #173 #202

[qa] Added linting of JS and CSS #173 #202

Conversation

niteshsinha17 commented May 27, 2021

atb00ker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niteshsinha17 commented May 31, 2021

atb00ker commented May 31, 2021

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niteshsinha17 Jun 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atb00ker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atb00ker left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niteshsinha17 Jun 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atb00ker Jun 5, 2021 • edited

Choose a reason for hiding this comment

niteshsinha17 Jun 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 7, 2021 • edited

niteshsinha17 commented Jun 7, 2021

atb00ker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niteshsinha17 commented Jun 8, 2021 • edited by nemesifier

Choose a reason for hiding this comment

niteshsinha17 Jun 2, 2021 •

edited

atb00ker left a comment •

edited

niteshsinha17 Jun 4, 2021 •

edited

atb00ker Jun 5, 2021 •

edited

niteshsinha17 Jun 5, 2021 •

edited

coveralls commented Jun 7, 2021 •

edited

niteshsinha17 commented Jun 8, 2021 •

edited by nemesifier