DATAUP-737: Second set of deepscan.io fixes #2889

ialarmedalien · 2022-04-12T23:59:08Z

Description of PR purpose/changes

Second set of of deep scan.io fixes. The majority of these modules have many eslint errors and couldn't be used in strict mode without terrible things happening.

Because there were a lot of issues raised by deepscan, I concentrated on fixing the 'undeclared global' errors and those that could easily be fixed by search and replace. The VSCode sonar cloud plugin highlights various errors like unused variables, and allows automatic replacement of array iteration using for (let i = 0; i < arr.length; i++) with for/of loops, so I fixed those and as many of the eslint issues as I could do without spending too much time on it. Most of these files still have problems due to undeclared variables or referencing modules/classes that appear to be in the global scope.

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/DATAUP-737

Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

Details for how to test the PR:

Tests pass locally and in GitHub Actions
Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
(JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook

ialarmedalien · 2022-04-13T00:00:59Z

kbase-extension/static/kbase/js/widgets/function_input/kbaseDefaultNarrativeInput.js

-                    if ($field.is('input') && $field.attr('type') === 'text') {
-                        $field.val(state[fieldName]);
-                    }
-
-                    // If it's a select field, do the same... we'll have comboboxen or something,
-                    // eventually, so I'm just leaving this open for that.
-                    else if ($field.is('select')) {


these do the same thing

ialarmedalien · 2022-04-13T00:04:45Z

kbase-extension/static/kbase/js/widgets/function_input/kbaseDefaultNarrativeInput.js

@@ -233,9 +232,9 @@ define(['kbwidget', 'bootstrap', 'jquery', 'narrativeConfig', 'kbaseNarrativeInp
                                $datalist = $(this.$elem.find('#' + datalistID));
                            }
                            $datalist.empty();
-                            for (let j = 0; j < objList.length; j++) {
+                            for (const element of objList) {


sonarcloud substitution

ialarmedalien · 2022-04-13T00:06:16Z

kbase-extension/static/kbase/js/widgets/function_output/contigBrowserPanel.js

-                            feature_location[i][2] === '+'
-                                ? start + length - 1
-                                : start - length + 1;
+                    for (const element of feature_location) {


sonarcloud substitution

ialarmedalien · 2022-04-13T00:06:42Z

kbase-extension/static/kbase/js/widgets/function_output/contigBrowserPanel.js

-                            feature_location[i][2] === '+'
-                                ? start + length - 1
-                                : start - length + 1;
+                    for (const element of feature_location) {


sonarcloud substitution

lgtm-com · 2022-04-13T00:07:08Z

This pull request introduces 5 alerts and fixes 105 when merging 9b77b83 into f1075d1 - view on LGTM.com

new alerts:

4 for Property access on null or undefined
1 for Superfluous trailing arguments

fixed alerts:

55 for Missing variable declaration
36 for Unused variable, import, function or class
5 for Useless assignment to local variable
2 for Unreachable statement
2 for Variable not declared before use
2 for Overwritten property
1 for Superfluous trailing arguments
1 for Comparison between inconvertible types
1 for Duplicate character in character class

codecov · 2022-04-13T00:17:03Z

Codecov Report

Merging #2889 (69ef736) into develop (7d10bf5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2889   +/-   ##
========================================
  Coverage    73.24%   73.24%           
========================================
  Files           36       36           
  Lines         3899     3899           
========================================
  Hits          2856     2856           
  Misses        1043     1043

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 f1075d1...69ef736. Read the comment docs.

ialarmedalien · 2022-04-13T15:04:54Z

kbase-extension/static/kbase/js/kbwidget.js

@@ -59,14 +59,14 @@ return KBWidget(

 Instantiate as follows:

-var $fancy = $('some-jquery-selector').myFancyNewWidget(
+const $fancy = $('some-jquery-selector').myFancyNewWidget(


Did a big search for var and replaced them all with let, and then ran eslint --fix, which caught some of the lets that could be changed to consts. Fixed the rest by hand.

ialarmedalien · 2022-04-13T15:06:17Z

...n/static/kbase/js/widgets/function_input/parameter_input/kbaseNarrativeParameterTextInput.js

@@ -897,13 +898,10 @@ define([
            switch (this.validateAs) {
                default:
                    return value;
-                    break;


don't need return and break

ialarmedalien · 2022-04-13T15:06:37Z

...n/static/kbase/js/widgets/function_input/parameter_input/kbaseNarrativeParameterTextInput.js

                if (!inputMapping) {
                    inputMapping = methodSpec['behavior']['script_input_mapping'];
-                    isScript = true;


this variable never gets used

ialarmedalien · 2022-04-13T15:07:48Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseAssembly.js

-                    //// FIX: modules are tagged, rather than categorized
-                    //          deferred.resolve([pre, asms, post]);
+                    const all = JSON.parse(mod_avail);
+                    // const pre = [],


none of these vars were used so I commented the code out

ialarmedalien · 2022-04-13T15:08:32Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseBlastOutput.js

@@ -271,12 +261,6 @@ define([
                    width = 540 - margin.left - margin.right,
                    height = 500 - margin.top - margin.bottom;

-                const padding = margin.left + margin.right;


ialarmedalien · 2022-04-13T15:11:54Z

...c/kbase/js/widgets/function_input/parameter_input/kbaseNarrativeParameterTextSubdataInput.js

                        const display =
                            '<span class="kb-parameter-data-selection">' + object.id + '</span>';
                        return display;
                    },
-                    formatResult: function (object, container, query) {


remove unused variables

ialarmedalien · 2022-04-14T14:27:25Z

kbase-extension/static/kbase/js/kbwidget.js

-            let arg;
-            for (arg in args) {


ialarmedalien · 2022-04-14T14:28:30Z

kbase-extension/static/kbase/js/widgets/function_input/kbStandaloneListselect.js

@@ -140,7 +140,7 @@

        render: function (index) {
            const renderer = rendererListselect[index];
-
+            let button_span, result_list;


need to be declared at this level as the previous code took advantage of variable hoisting

ialarmedalien · 2022-04-14T14:29:03Z

kbase-extension/static/kbase/js/widgets/function_input/kbStandaloneListselect.js

-                    ? 'max-height: 200px; overflow: auto;'
-                    : 'max-height: 200px; overflow: auto;'


I love me some ternaries, but this one seems a little unnecessary...

ialarmedalien · 2022-04-14T14:29:53Z

...atic/kbase/js/widgets/function_input/parameter_input/kbaseNarrativeParameterCheckboxInput.js

            }
-            return '';


never reached

ialarmedalien · 2022-04-14T14:30:43Z

...n/static/kbase/js/widgets/function_input/parameter_input/kbaseNarrativeParameterTextInput.js

@@ -686,7 +686,7 @@ define([
                                        'data object names cannot be a number, in field ' +
                                            self.spec.ui_name
                                    );
-                                } else if (!/^[a-z0-9|\.|\||_\-]*$/i.test(pVal)) {


incorrect syntax for a character range

ialarmedalien · 2022-04-14T14:32:16Z

kbase-extension/static/kbase/js/kbwidget.js

@@ -27,7 +27,7 @@ return KBWidget(
        version : "1.0.0",  //future proofing, but completely unused

        _accessors : [  //optional. A list of values to automatically create setter/getters for.
-            'foo'       //you'll now be able to store something at $widget.foo('newValue') and access it via var foo = $widget.foo();


Generally I have tried to add comments whenever there's a change that isn't var -> let/const, removing an unused argument, renaming a var that is used in the upper scope, or something else that's similarly trivial.

ialarmedalien · 2022-04-14T14:34:55Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseAssembly.js

@@ -92,37 +91,28 @@ define(['kbwidget', 'bootstrap', 'jquery', 'kbasePrompt'], (
                return prom;
            };

-            const get_job_report_log = function (job_id) {


ialarmedalien · 2022-04-14T14:36:02Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseAssembly.js

+            _token,
+            _fba_url,
+            _ws_url,


args unused => prefix them with _ so that linters don't complain

ialarmedalien · 2022-04-14T14:44:36Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseGenomeComparisonViewer.js

@@ -349,7 +338,7 @@ define([
                    );
                    tabFunctions.append(tableFunctions);
                    const func_data = [];
-                    var tableSettings = {
+                    const tableTwoSettings = {


reused var name ==> change the name to something new and very original!

ialarmedalien · 2022-04-14T14:46:45Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseModelCoreNarrative.js

-                        const rect = stage.core_rect(obj, x, y, r_width, r_height, show_flux);
-                        //var text = stage.text(x+r_width/2+50, y+r_height/2, obj.rxns.join('\n'))
-                        //text.attr({'font-size':8})
+                        // rect = stage.core_rect(obj, x, y, r_width, r_height, show_flux);


These vars never get used so commented out (for now). Who knows what is going on in this code... maybe they will be useful to a hypothetical future code reader.

ialarmedalien · 2022-04-14T14:47:11Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseModelCoreNarrative.js

-                    var y3 = (xys[2][1] - 2) * grid + radius * 3;
+                    x1 = (xys[0][0] - 2) * grid + r_width / 2;
+                    y1 = (xys[0][1] - 2) * grid + radius * 3;
+                    // x2 = (xys[1][0] - 2) * grid + r_width / 2;


x2 is not used

ialarmedalien · 2022-04-14T14:50:05Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseSeqCompView.js

@@ -86,14 +74,14 @@ define([
                        // kbws.get_object_subset([{ref: 'fangfang:1452395167784' +"/"+ 'Rhodobacter_CACIA_14H1_contigs', included: ['contigs/[*]/id', 'contigs/[*]/length', 'id', 'name', 'source', 'source_id', 'type']}], function(data) {

                        container.empty();
-                        var data = data[0].data;
-                        console.log(data);
+                        const dataData = data[0].data;


data already used in upper scope, so I'm calling this dataData to reflect its origins

ialarmedalien · 2022-04-14T14:50:21Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseUploadFile.js

@@ -58,11 +58,6 @@
                            );
                            return 1;
                        }
-                    } else if (typeof params.externalProgressTarget == 'undefined') {


never called

ialarmedalien · 2022-04-14T14:50:55Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseUploadFile.js

@@ -129,7 +124,7 @@
                    error.statusText +
                    '\
 </div>';
-                if (typeof uploader.callback == 'function') {


uploader is not defined -- guessed it probably mean ul, which is the abbrev used elsewhere

ialarmedalien · 2022-04-14T14:51:27Z

kbase-extension/static/kbase/js/widgets/function_output/modeling/KBaseFBA.FBAComparison.js

-            this.modelreactions[i]['name'] = namearray[0];
-            this.modelreactions[i].dispid = idarray[0] + '[' + idarray[1] + ']';
-            this.rxnhash[this.modelreactions[i].id] = this.modelreactions[i];
+        for (const element of this.modelreactions) {


sonar cloud auto substitution

ialarmedalien · 2022-04-14T14:52:04Z

kbase-extension/static/kbase/js/widgets/function_output/modeling/KBaseFBA.FBAComparison.js

                sign = '<=';
            }
-            for (var j = 0; j < this.modelreactions[i].stoichiometry.length; j++) {
-                const rgt = this.modelreactions[i].stoichiometry[j];
+            for (let j = 0; j < element.stoichiometry.length; j++) {


unfortunately sonar cloud can't do nested for-of loops

ialarmedalien · 2022-04-14T14:53:19Z

kbase-extension/static/kbase/js/widgets/kbaseButtonControls.js

@@ -113,9 +114,6 @@ define([
            this._rewireIds($elem, this);

            if (this.options.onMouseover && this.options.type == 'floating') {
-                const overControls = false;


lots of unused vars in this file

lgtm-com · 2022-04-14T15:21:50Z

This pull request introduces 4 alerts and fixes 104 when merging 69ef736 into d6ff0d5 - view on LGTM.com

new alerts:

4 for Property access on null or undefined

fixed alerts:

55 for Missing variable declaration
35 for Unused variable, import, function or class
5 for Useless assignment to local variable
2 for Unreachable statement
2 for Variable not declared before use
2 for Overwritten property
1 for Superfluous trailing arguments
1 for Comparison between inconvertible types
1 for Duplicate character in character class

sonarqubecloud · 2022-04-14T15:29:03Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
72 Code Smells

No Coverage information
6.7% Duplication

briehl

This all looks pretty good to me, even tinkering with ancient code.
I'm a little nervous about sweeping changes to the data viewers - even minor structural changes - without testing or tinkering. I'd like to just have some quick data checks to make sure these still work as expected.

Also, I'm pretty sure there are several that aren't used or associated with any app. Though I'm not sure how they get used or invoked, then. I think the kbaseAssembly and kbaseModelCoreNarrative are a couple examples of that.

I think as long as we can show that they're structurally identical, though, this is probably ok to merge and break everything on CI, then apply spot fixes from there.

briehl · 2022-04-14T23:20:28Z

kbase-extension/static/kbase/js/widgets/function_output/kbaseAssembly.js

@@ -290,9 +278,9 @@ define(['kbwidget', 'bootstrap', 'jquery', 'kbasePrompt'], (
                '<button class="btn btn-default col-md-2 btn-primary pull-right asm-form"><span class="glyphicon glyphicon-play-circle"  style="color:white;"></span> Assemble</button>'
            );

-            var update_asm_pool = function (assembler_pool, pool) {
+            const update_asm_pool = function (ass_pool, pool) {


You did that on purpose. 😈

possibly...

ialarmedalien self-assigned this Apr 12, 2022

ialarmedalien requested a review from briehl April 12, 2022 23:59

ialarmedalien commented Apr 13, 2022

View reviewed changes

ialarmedalien commented Apr 14, 2022

View reviewed changes

kbase-extension/static/kbase/js/kbwidget.js

Comment on lines -618 to -619

let arg;

for (arg in args) {

Copy link

Collaborator Author

ialarmedalien Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

combined

ialarmedalien commented Apr 14, 2022

View reviewed changes

Fixing deepscan, sonarcloud, and other linting issues

69ef736

ialarmedalien force-pushed the DATAUP-737_deepscan_fixes_II branch from 9b77b83 to 69ef736 Compare April 14, 2022 15:13

briehl reviewed Apr 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAUP-737: Second set of deepscan.io fixes #2889

DATAUP-737: Second set of deepscan.io fixes #2889

ialarmedalien commented Apr 12, 2022 •

edited

Loading

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

lgtm-com bot commented Apr 13, 2022

codecov bot commented Apr 13, 2022 •

edited

Loading

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

ialarmedalien Apr 13, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022 •

edited

Loading

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

ialarmedalien Apr 14, 2022

lgtm-com bot commented Apr 14, 2022

sonarqubecloud bot commented Apr 14, 2022

briehl left a comment

briehl Apr 14, 2022

ialarmedalien Apr 15, 2022

		? 'max-height: 200px; overflow: auto;'
		: 'max-height: 200px; overflow: auto;'

DATAUP-737: Second set of deepscan.io fixes #2889

Are you sure you want to change the base?

DATAUP-737: Second set of deepscan.io fixes #2889

Conversation

ialarmedalien commented Apr 12, 2022 • edited Loading

Description of PR purpose/changes

Jira Ticket / Issue

Testing Instructions

Dev Checklist:

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

lgtm-com bot commented Apr 13, 2022

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

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

ialarmedalien Apr 14, 2022 • 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

lgtm-com bot commented Apr 14, 2022

sonarqubecloud bot commented Apr 14, 2022

briehl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ialarmedalien commented Apr 12, 2022 •

edited

Loading

codecov bot commented Apr 13, 2022 •

edited

Loading

ialarmedalien Apr 14, 2022 •

edited

Loading