eslint-plugin-sql icon indicating copy to clipboard operation
eslint-plugin-sql copied to clipboard

Add matchIndentation option

Open karlhorky opened this issue 4 years ago • 12 comments

Closes https://github.com/gajus/eslint-plugin-sql/issues/9

karlhorky avatar Nov 20 '20 19:11 karlhorky

@gajus any thoughts on this one?

karlhorky avatar Nov 26 '20 11:11 karlhorky

Maybe. Needs tests. Too early to give feedback.

gajus avatar Nov 26 '20 12:11 gajus

Hmm... looking at https://github.com/gajus/eslint-plugin-sql/blob/master/test/rules/assertions/format.js it seems like there is no plumbing for the template string code around the SQL.

Could you offer me some guidance as to how you would suggest implementing tests for this feature?

karlhorky avatar Nov 26 '20 13:11 karlhorky

Ah looks like there could be some edge cases here with indentation, frigus went and worked through them in his PR here:

https://github.com/frigus02/typescript-sql-tagged-template-plugin/pull/14

Maybe if the AST method isn't working, manually counting the indentation will be required.

karlhorky avatar Nov 28 '20 15:11 karlhorky

Another bit of inspiration can probably be found in Prettier's tagged template string indentation:

https://github.com/prettier/prettier/blob/2247ce1aab604bf1b95b3d010c08d48b003286c7/src/language-js/embed.js#L164

karlhorky avatar Jan 23 '21 21:01 karlhorky

I am down to merge this, but it would need pretty thorough tests, just to make sure that we don't introduce hard to debug bugs.

gajus avatar Jan 25 '21 20:01 gajus

Nice, cheers. I may have some time soon to rewrite the implementation, based on the "inspiration" links above.

As for the tests, would you be able to offer any guidance for a method for testing this?

karlhorky avatar Jan 27 '21 09:01 karlhorky

As for the tests, would you be able to offer any guidance for a method for testing this?

If testing integration is hard, then I would just abstract any fragile logic into a utility and test that. Otherwise, leave it without a test.

gajus avatar Jan 28 '21 00:01 gajus

Thanks you guys for this great tools and improvements, waiting for this PR merged with impatience ;)

devthejo avatar Nov 05 '21 11:11 devthejo

@devthejo not sure I'll have time soon to look at this soon, maybe you could contribute to the feature?

karlhorky avatar Nov 05 '21 12:11 karlhorky

@karlhorky sorry I have not enough time to make tests, but I've refactored a little bit, removing the bug that was adding new line #9, and removing option ignoreStartWithNewLine replacing with adding a new line at start by default, so I have, for example:

sql`
const row = await conn.one(
  sql`
    SELECT
      "user_id" as "userId",
      "device_id" as "deviceId"
    FROM
      "auth_token"
    WHERE
      "auth_token" = ${authToken}
    `
)

If this can be useful for someone I put the code here: format.js

// @flow

import {
  generate,
} from 'astring';
import {
  format,
} from 'pg-formatter';
import isSqlQuery from '../utilities/isSqlQuery';

export default (context) => {
  const placeholderRule = context.settings.sql && context.settings.sql.placeholderRule;

  const pluginOptions = context.options && context.options[0] || {};

  const ignoreExpressions = pluginOptions.ignoreExpressions === true;
  const ignoreInline = pluginOptions.ignoreInline !== false;
  const ignoreTagless = pluginOptions.ignoreTagless !== false;
  const matchIndentation = pluginOptions.matchIndentation !== false;

  const pgFormatterOptions = context.options && context.options[1] || {};

  return {
    TemplateLiteral (node) {
      const sqlTagIsPresent = node.parent.tag && node.parent.tag.name === 'sql';

      if (ignoreTagless && !sqlTagIsPresent) {
        return;
      }

      if (ignoreExpressions && node.quasis.length !== 1) {
        return;
      }

      const magic = '"gajus-eslint-plugin-sql"';

      const literal = node.quasis
        .map((quasi) => {
          return quasi.value.raw;
        })
        .join(magic);

      if (!sqlTagIsPresent && !isSqlQuery(literal, placeholderRule)) {
        return;
      }

      if (ignoreInline && !literal.includes('\n')) {
        return;
      }

      let formatted = format(literal, context.options[1]);

      formatted = formatted.trimStart();

      if (matchIndentation) {
        let firstNodeOnLine = node;

        while (
          firstNodeOnLine.parent &&
          firstNodeOnLine.loc.start.line === firstNodeOnLine.parent.loc.start.line
        ) {
          firstNodeOnLine = firstNodeOnLine.parent;
        }

        const startingColumn = firstNodeOnLine.loc.start.column;
        formatted = formatted.replace(/\n+$/g, '') + '\n';
        const formattedLines = formatted.split('\n');
        formatted = formattedLines
          .map((line) => {
            // Indent each subsequent line based on the spaces option
            const indentSpaces = context.options[1].spaces || 4;

            const indentation = ' '.repeat(startingColumn + indentSpaces);

            return `${indentation}${line}`;
          })
          .join('\n');
      }

      formatted = '\n' + formatted.replace(/^\n+/g, '');

      if (formatted !== literal) {
        context.report({
          fix: (fixer) => {
            let final = formatted;

            const expressionCount = node.expressions.length;
            let index = 0;
            while (index <= expressionCount - 1) {
              final = final.replace(magic, '${' + generate(node.expressions[index]) + '}');
              index++;
            }

            return fixer.replaceText(node, '`' + final + '`');
          },
          message: 'Format the query',
          node,
        });
      }
    },
  };
};

devthejo avatar Nov 05 '21 17:11 devthejo

Will it be merged?

ssukienn avatar Jun 03 '22 18:06 ssukienn

Would need a rebase, but otherwise happy to accept it.

gajus avatar Oct 18 '22 13:10 gajus

@gajus What happened to this? It looks like you closed it but never did a rebase or a merge.

vantaboard avatar Jan 26 '23 08:01 vantaboard

From my side, I didn't find the time to work on it yet. But maybe someone else can take over and (re-)implement in a new PR?

karlhorky avatar Jan 26 '23 09:01 karlhorky

Went ahead and did it based off of your code @karlhorky https://github.com/gajus/eslint-plugin-sql/pull/25

vantaboard avatar Jan 26 '23 14:01 vantaboard

Amazing, thanks! Would be great to see this merged!

karlhorky avatar Jan 26 '23 15:01 karlhorky

What do you think @gajus ?

vantaboard avatar Feb 01 '23 11:02 vantaboard