Add `npm run quality` script, Update Readme #97

mukkachaitanya · 2018-10-19T05:20:14Z

Requirements

Need for easy and better checks for code quality.

Description of the Change

Adds npm run quality script that uses eslint, jsinspect and buddy.js to keep a check on the quality of the project.

Benefits

Improved and easy code quality checking.

Applicable Issues

Addresses #96

codecov · 2018-10-19T05:21:44Z

Codecov Report

Merging #97 into dev will decrease coverage by 1.41%.
The diff coverage is 88.15%.

@@           Coverage Diff            @@
##            dev      #97      +/-   ##
========================================
- Coverage   100%   98.58%   -1.42%     
========================================
  Files        21       22       +1     
  Lines       591      635      +44     
========================================
+ Hits        591      626      +35     
- Misses        0        9       +9

Flag	Coverage Δ
#integration	`95.74% <88.15%> (-1.21%)`	⬇️
#unit	`97.79% <86.84%> (-2.04%)`	⬇️

Impacted Files	Coverage Δ
lib/cli/input/init.js	`100% <ø> (ø)`	⬆️
lib/cli/output/exit.js	`90% <0%> (-10%)`	⬇️
lib/model/prefs.js	`100% <100%> (ø)`	⬆️
lib/controller/index.js	`100% <100%> (ø)`
lib/model/init.js	`100% <100%> (ø)`	⬆️
lib/utils/logger.js	`100% <100%> (ø)`	⬆️
lib/cli/input/eval.js	`100% <100%> (ø)`	⬆️
lib/utils/command-validator.js	`100% <100%> (ø)`	⬆️
lib/model/eval.js	`97.56% <66.66%> (-2.44%)`	⬇️
lib/cli/input/prefs.js	`96.82% <75%> (-3.18%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff0d6a0...417c17c. Read the comment docs.

prasadtalasila

@mukkachaitanya please see the suggested changes.

prasadtalasila · 2018-10-19T06:46:01Z

package.json

@@ -9,7 +9,8 @@
    "integration": "nyc --reporter=lcov --reporter=text-lcov -s --all mocha 'test/integration/**/*.js'",
    "post-integration": "nyc report --reporter=lcov > coverage.lcov && codecov -F integration && rm -rf ~/.autolabjs",
    "feature": "nyc --reporter=lcov --reporter=text-lcov -s --all cucumber-js 'test/feature/features' -r 'test/feature/steps'",
-    "post-feature": "nyc report --reporter=lcov > coverage.lcov && codecov -F feature"
+    "post-feature": "nyc report --reporter=lcov > coverage.lcov && codecov -F feature",
+    "quality" : "eslint . & jsinspect . & buddy ."


now the style is to put a comma even on the last line so that the diff shows modification only on one line.

"quality" : "eslint . && jsinspect . && buddy ."

Sir, if eslint fails the none of the further checks are taken, i.e., none of jsinspect/buddy commands are executed.

Also sir, json doesn't accept a trailing comma. Throws the following error :

npm ERR! JSON.parse Unexpected token } in JSON at position 831 while parsing near '...ect . & buddy .", npm ERR! JSON.parse }, npm ERR! JSON.parse "bin": { npm ERR! JSON.parse "a...' npm ERR! JSON.parse Failed to parse package.json data. npm ERR! JSON.parse package.json must be actual JSON, not just JavaScript.

prasadtalasila · 2018-10-19T06:50:47Z

.eslintrc.json

@@ -1,5 +1,9 @@
 {
    "extends": "airbnb-base",
+    "env" : {


why are these needed?

eslint give errors of the form :

12:3 error 'it' is not defined no-undef 18:3 error 'describe' is not defined no-undef

when mocha env is not set to be true.

prasadtalasila · 2018-10-19T06:53:40Z

.jsinspectrc

@@ -0,0 +1,8 @@
+{
+  "threshold":     60,


This is not similar to codeclimate duplication weight. This number is the number of lines. A prudent value seems to be between 4 to 6.

I'll make that change.

prasadtalasila · 2018-10-19T06:55:12Z

@mukkachaitanya also it is better to run npm quality command in the travis after the tests run successfully.

mukkachaitanya · 2018-10-19T07:31:15Z

Also Sir, there are a lot of eslint errors which have to be taken care of. I don't see codeclimate reporting them.

prasadtalasila · 2018-10-19T13:18:18Z

Also Sir, there are a lot of eslint errors which have to be taken care of. I don't see codeclimate reporting them.

This is rude awakening for me as well. Please fix these issues before we do further development. One hypersensitive check is the threshold parameter in .jsinspectrc. This number can be increased to 20.

mukkachaitanya · 2018-10-26T14:22:50Z

Some changes in commit 6db57af to be considered :

Added buddyrc file to configure the buddy.js tool, which ignores [0, 1, 2, 3] as constants as we use them in the array indexing.
The threshold for jsinspect has been changed to the default value 30 as it was reporting issuses which we discussed to not to be limit.
The following rule was modified to allow the usage of unary ++ or -- operators in loops only.

"no-plusplus" : ["error", { "allowForLoopAfterthoughts": true }],

Also, the following rule was added to take care of issues arising from cumumber.js tests

"no-unused-vars": [
            "error",
            {
              "varsIgnorePattern": "should|expect|Before|After|Given"
            }
        ]

The following rule also switches of max-lines-per-function in the unit tests

 "max-lines-per-function" : ["off"]

Also the the rule no-reassign-param has been switched off in some places especially in the integration test. (for example, test/integration/eval.js)

prasadtalasila · 2018-10-26T14:44:35Z

.buddyrc

+{
+  "detectObjects": false,
+  "enforceConst":  false,
+  "ignore":        [0, 1, 2, 3], 


please remove this ignore statement.

prasadtalasila · 2018-10-26T14:45:35Z

.buddyrc

@@ -0,0 +1,6 @@
+{
+  "detectObjects": false,


please clarify the use of next two lines.

According to the documentation :

detectObjects - detect object expressions and properties enforceConst - require literals to be defined using const

I think we can switch both to true.

prasadtalasila · 2018-10-26T14:50:33Z

.eslintrc

+        "node": true,
+        "mocha": true
+    },
+    "rules": {


please clarify the reason for using the rule.

prasadtalasila · 2018-10-26T15:02:08Z

.eslintrc

+        "no-unused-vars": [
+            "error",
+            {
+              "varsIgnorePattern": "should|expect|Before|After|Given"


avoid this rule using a plug-in?

prasadtalasila · 2018-10-26T15:14:00Z

lib/cli/output/eval.js

@@ -37,9 +41,9 @@ const printTableAndGetScore = (data) => {
 };

 const printStatusAndLog = (data, totalScore) => {
-  logger.moduleLog('debug', 'Eval Output', `Evaluation logs: ${new Buffer(data.log, 'base64').toString()}.`);
+  logger.moduleLog('debug', 'Eval Output', `Evaluation logs: ${Buffer.from(data.log, 'base64').toString()}.`);


The Buffer constructor is different depending on the type of its argument, and not validating such arguments might lead to memory disclosure and denial of service. So eslint enforces the use of Buffer.from() or Buffer.alloc() instead.

ref

prasadtalasila · 2018-10-26T15:14:20Z

lib/cli/input/prefs.js

  if (!type) {
    const typePrompt = prefPrompts.getServerTypePrompt();
-    type = (await inquirer.prompt(typePrompt)).type;
+    ({ type } = (await inquirer.prompt(typePrompt)));


why brackets?

We have discussed this, sir, but for having everything in a single thread; the MDN documentation says:

The parentheses ( ... ) around the assignment statement are required when using object literal destructuring assignment without a declaration. {a, b} = {a: 1, b: 2} is not valid stand-alone syntax, as the {a, b} on the left-hand side is considered a block and not an object literal. However, ({a, b} = {a: 1, b: 2}) is valid, as is var {a, b} = {a: 1, b: 2} NOTE: Your ( ... ) expression needs to be preceded by a semicolon or it may be used to execute a function on the previous line.

prasadtalasila · 2018-10-26T15:22:56Z

lib/model/prefs.js

@@ -77,7 +80,7 @@ const storePrefs = (changePrefEvent) => {
  } else if (changePrefEvent.name === 'server_changed') {
    storeServer(changePrefEvent.details);
  } else if (changePrefEvent.name === 'show_prefs') {
-    changePrefEvent = eventForShowPref(changePrefEvent);
+    return eventForShowPref(changePrefEvent);


The function storePrefs was modifying the input argument changePrefEvent storing and then returning it. Instead, the above modification returns the new value directly.

prasadtalasila · 2018-10-26T15:23:59Z

package.json

@@ -9,7 +9,8 @@
    "integration": "nyc --reporter=lcov --reporter=text-lcov -s --all mocha 'test/integration/**/*.js'",
    "post-integration": "nyc report --reporter=lcov > coverage.lcov && codecov -F integration && rm -rf ~/.autolabjs",
    "feature": "nyc --reporter=lcov --reporter=text-lcov -s --all cucumber-js 'test/feature/features' -r 'test/feature/steps'",
-    "post-feature": "nyc report --reporter=lcov > coverage.lcov && codecov -F feature"
+    "post-feature": "nyc report --reporter=lcov > coverage.lcov && codecov -F feature",
+    "quality": "eslint . & jsinspect . & buddy ."


please add &&

mukkachaitanya · 2018-10-26T15:41:13Z

lib/utils/preference-manager.js

@@ -1,3 +1,4 @@
+/* eslint no-use-before-define: "warn" */


Need to refactor the cyclic functions.

Add npm run quality script, Modify Readme

11d14f7

prasadtalasila requested changes Oct 19, 2018

View reviewed changes

mukkachaitanya force-pushed the code-quality branch 2 times, most recently from 6384353 to 254c706 Compare October 19, 2018 08:16

Modifie travis build to check quality

e99c3b6

mukkachaitanya force-pushed the code-quality branch from 254c706 to e99c3b6 Compare October 19, 2018 08:29

Solves quality issues

6db57af

prasadtalasila requested changes Oct 26, 2018

View reviewed changes

mukkachaitanya commented Oct 26, 2018

View reviewed changes

More modifications to improve quality

417c17c

mukkachaitanya force-pushed the code-quality branch from b311e6a to 417c17c Compare October 31, 2018 15:58

prasadtalasila merged commit 298bfd8 into AutolabJS:dev Oct 31, 2018

snyk-bot mentioned this pull request Sep 14, 2021

[Snyk] Security upgrade caporal from 0.9.0 to 1.0.0 #139

Open

prasadtalasila mentioned this pull request May 17, 2022

[Snyk] Security upgrade caporal from 0.9.0 to 1.0.0 #142

Open

prasadtalasila mentioned this pull request Nov 28, 2023

[Snyk] Security upgrade caporal from 0.9.0 to 1.0.0 #149

Open

prasadtalasila mentioned this pull request Apr 17, 2024

[Snyk] Fix for 1 vulnerabilities #151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `npm run quality` script, Update Readme #97

Add `npm run quality` script, Update Readme #97

mukkachaitanya commented Oct 19, 2018

codecov bot commented Oct 19, 2018 •

edited

Loading

prasadtalasila left a comment

prasadtalasila Oct 19, 2018

prasadtalasila Oct 19, 2018

mukkachaitanya Oct 19, 2018

mukkachaitanya Oct 19, 2018

prasadtalasila Oct 19, 2018

mukkachaitanya Oct 19, 2018

prasadtalasila Oct 19, 2018

mukkachaitanya Oct 19, 2018

prasadtalasila commented Oct 19, 2018

mukkachaitanya commented Oct 19, 2018

prasadtalasila commented Oct 19, 2018

mukkachaitanya commented Oct 26, 2018 •

edited

Loading

prasadtalasila Oct 26, 2018

prasadtalasila Oct 26, 2018

mukkachaitanya Oct 29, 2018

prasadtalasila Oct 26, 2018

prasadtalasila Oct 26, 2018

prasadtalasila Oct 26, 2018

mukkachaitanya Oct 29, 2018

prasadtalasila Oct 26, 2018

mukkachaitanya Oct 29, 2018

prasadtalasila Oct 26, 2018

mukkachaitanya Oct 29, 2018

prasadtalasila Oct 26, 2018

mukkachaitanya Oct 26, 2018

Add npm run quality script, Update Readme #97

Add npm run quality script, Update Readme #97

Conversation

mukkachaitanya commented Oct 19, 2018

Requirements

Description of the Change

Benefits

Applicable Issues

codecov bot commented Oct 19, 2018 • edited Loading

Codecov Report

prasadtalasila 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prasadtalasila commented Oct 19, 2018

mukkachaitanya commented Oct 19, 2018

prasadtalasila commented Oct 19, 2018

mukkachaitanya commented Oct 26, 2018 • edited Loading

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

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

Add `npm run quality` script, Update Readme #97

Add `npm run quality` script, Update Readme #97

codecov bot commented Oct 19, 2018 •

edited

Loading

mukkachaitanya commented Oct 26, 2018 •

edited

Loading