Skip to content

Add matchIndentation option #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

karlhorky
Copy link

Closes #9

@karlhorky
Copy link
Author

@gajus any thoughts on this one?

@gajus
Copy link
Owner

gajus commented Nov 26, 2020

Maybe. Needs tests. Too early to give feedback.

@karlhorky
Copy link
Author

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
Copy link
Author

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

frigus02/typescript-sql-tagged-template-plugin#14

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

@karlhorky
Copy link
Author

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

@gajus
Copy link
Owner

gajus commented Jan 25, 2021

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.

@karlhorky
Copy link
Author

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?

@gajus
Copy link
Owner

gajus commented Jan 28, 2021

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.

@devthejo
Copy link

devthejo commented Nov 5, 2021

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

@karlhorky
Copy link
Author

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

@devthejo
Copy link

devthejo commented Nov 5, 2021

@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,
        });
      }
    },
  };
};

@ssukienn
Copy link

ssukienn commented Jun 3, 2022

Will it be merged?

@gajus
Copy link
Owner

gajus commented Oct 18, 2022

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

@gajus gajus closed this Oct 18, 2022
@vantaboard
Copy link

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

@karlhorky
Copy link
Author

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?

@vantaboard
Copy link

vantaboard commented Jan 26, 2023

Went ahead and did it based off of your code @karlhorky #25

@karlhorky
Copy link
Author

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

@vantaboard
Copy link

What do you think @gajus ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option for format rule for matching indentation?
5 participants