seegno / jscs-config-seegno

Seegno-flavored JSCS config
2 stars 0 forks source link

Replace `requireSqlTemplateInQueryFunction` rule #14

Closed ruiquelhas closed 8 years ago

ruiquelhas commented 8 years ago

Resolves #13.

ruimarinho commented 8 years ago

LGTM! It would be good to get @rplopes' confirmation too.

rplopes commented 8 years ago

While testing this, I'm getting unsupported rule:

Unsupported rule: requireSqlTemplate at X :
     1 |'use strict';
--------^
     2 |
     3 |/**

Is there something I'm missing? I've changed my package.json to use:

"jscs": "2.6.0",
"jscs-config-seegno": "seegno/jscs-config-seegno#enhancement/broader-sql-template-query-enforcement",

EDIT: After building the dependency, I got it to run as expected. However, I got some false positives, like:

Use the `sql` tagged template literal for raw queries at X :
   114 |  });
   115 |
   116 |  describe('from non-members to members', function() {
-------------------^
   117 |    for (const from of currencies) {
   118 |      for (const to of currencies) {

I also got errors for use-strict:

Error running rule requireSqlTemplate: This is an issue with JSCS and not your codebase.
Please file an issue (with the stack trace below) at: https://github.com/jscs-dev/node-jscs/issues/new
TypeError: query.replace is not a function
    at Object.exports.sql2ast (/Users/ricardo/Projects/backend/node_modules/jscs-config-seegno/node_modules/simple-sql-parser/simpleSqlParser.js:59:17)
    at isSQLQuery (/Users/ricardo/Projects/backend/node_modules/jscs-config-seegno/dist/rules/require-sql-template.js:28:62)
    at /Users/ricardo/Projects/backend/node_modules/jscs-config-seegno/dist/rules/require-sql-template.js:55:35
    at Array.forEach (native)
    at Object.<anonymous> (/Users/ricardo/Projects/backend/node_modules/jscs-config-seegno/dist/rules/require-sql-template.js:51:22)
    at Array.forEach (native)
    at Object.JsFile.iterateNodesByType (/Users/ricardo/Projects/backend/node_modules/jscs/lib/js-file.js:595:42)
    at Object.check (/Users/ricardo/Projects/backend/node_modules/jscs-config-seegno/dist/rules/require-sql-template.js:50:10)
    at null.<anonymous> (/Users/ricardo/Projects/backend/node_modules/jscs/lib/string-checker.js:165:22)
    at Array.forEach (native) at X :
     1 |'use strict';
--------^
     2 |
     3 |/**
ruiquelhas commented 8 years ago

The SQL parser was not working the way I expected. Should be working now (I switched to a different lib btw).

rplopes commented 8 years ago

This latest version is working perfectly :shipit:

ruiquelhas commented 8 years ago

ping @ruimarinho.

rplopes commented 8 years ago

Bump 🙂

ruimarinho commented 8 years ago

LGTM, just not sure about running an SQL parser on every literal. Might be too costly?

rplopes commented 8 years ago

Is there anything currently blocking this PR?

ruimarinho commented 8 years ago

@rplopes I have addressed my concerns on the PR and waiting for @ruiquelhas's feedback.

ruiquelhas commented 8 years ago

Sorry, the discussion got lost somewhere. @ruimarinho your questions were finally answered.

ruimarinho commented 8 years ago

After a non scientific benchmark was produced by @ruiquelhas against a relatively large code base, the performance impact of parsing every string literal as SQL is negligent, so this will be merged.

ruiquelhas commented 8 years ago

@ruimarinho bump.

ruimarinho commented 8 years ago

As discussed offline, we should only apply this rule when interpolation is used.

ruiquelhas commented 8 years ago

@ruimarinho the rule should now be working as discussed.

ruimarinho commented 8 years ago

This looks good! Nit: fixed the commit message casing.