Skip to content
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

Fixable rules #82

Open
sindresorhus opened this issue Aug 1, 2016 · 13 comments
Open

Fixable rules #82

sindresorhus opened this issue Aug 1, 2016 · 13 comments
Labels
🗄 area/interface This affects the public interface 🌊 blocked/upstream This cannot progress before something external happens first help wanted 🙏 This could use your insight or help 👀 no/external This makes more sense somewhere else 🧑 semver/major This is a change 🦋 type/enhancement This is great to have 🙆 yes/confirmed This is confirmed and ready to be worked on

Comments

@sindresorhus
Copy link

I'm aware I could use Remark to modify the Markdown, but I'm looking for a way to add autofixing to remark-lint rules, similar to ESLint.

See how ESLint solves it: http://eslint.org/docs/developer-guide/working-with-rules#applying-fixes

Could be a --fix flag to remark-lint CLI.

@wooorm
Copy link
Member

wooorm commented Aug 6, 2016

That would be really awesome! And it’s a pretty smart idea, to do it like that. Not entirely sure about al the methods though, why not just start/end position, and new text, so more like:

function finalNewline(ast, file) {
  var contents = file.toString();
  var last = contents.length - 1;
  var message;

  if (last > -1 && contents.charAt(last) !== '\n') {
    message = file.warn('Missing newline character at end of file');
    message.fix = [last, last, '\n']; // start offset, end offset, replacement text
  }
}

From what I gather, ESLint/xo allows running with --fix to apply all fixes. Does it also have a prompt base interface where users are asked whether to apply each fix? Something like:

> remark -u lint --fix-promt

Yields:

- asdasdasd
+ asdasdasd\n
Missing newline character at end of file
Want to apply this fix? (yN)

@wooorm wooorm added 🙉 open/needs-info This needs some more info 👀 no/external This makes more sense somewhere else labels Aug 6, 2016
@sindresorhus
Copy link
Author

Not entirely sure about al the methods though, why not just start/end position, and new text, so more like:

The ESLint fixer works on AST, not offsets. This one should do the same.

From what I gather, ESLint/xo allows running with --fix to apply all fixes.

It tries to apply as many fixes as possible:

Note that the fix is not immediately applied and may not be applied at all if there are conflicts with other fixes. If the fix cannot be applied, then the problem message is reported as usual; if the fix can be applied, then the problem message is not reported. - http://eslint.org/docs/developer-guide/working-with-rules#applying-fixes

Does it also have a prompt base interface where users are asked whether to apply each fix?

No. The goal should be to just fix everything without user interaction. I guess it could prompt for ambiguous cases.

I would strongly recommend digging into more deeply how ESLint does it. Discussions in the issue tracker, etc. They've spent a lot of time on it.

@wooorm
Copy link
Member

wooorm commented Aug 6, 2016

The ESLint fixer works on AST, not offsets. This one should do the same.

Not really, the fix() function returns a {range: [index, index], text: text} (through those helper functions), you can also see them in the json formatter.

No. The goal should be to just fix everything without user interaction. I guess it could prompt for ambiguous cases.

That works really well for some of the rules, like inserting some white-space or a few characters, but not so much for bigger changes (as ESLint mentions in there docs), as they often interfere with other changes.

Plus it’d be cool for fenced-code-flag to try to detect the code language and suggest that, or alternatively use something the user types in!

@dirkroorda
Copy link

I do not use —fix because of the existence of prettier.
It seems prettier is also underway for markdown, using the remark-parser, but it currently lacks sufficient output options. It does do word wrap for long lines.
Currently, I use first prettier and then remark for fixing my markdown. It works on the command line, and from within vim, with a single keypress ( the ALE plugin).
So I think fixing based on individual rules might be a waste of time. But you do need a fair amount of options that control how to format an AST.

@wooorm wooorm added help wanted 🙏 This could use your insight or help 🌊 blocked/upstream This cannot progress before something external happens first 🗄 area/interface This affects the public interface 🙆 yes/confirmed This is confirmed and ready to be worked on 🦋 type/enhancement This is great to have 🧑 semver/major This is a change and removed 🙉 open/needs-info This needs some more info labels Aug 15, 2019
@muescha
Copy link

muescha commented Jul 23, 2020

is there a way to use the eslint ecosystem:

  • exporting remark-lint messages with fixes and suggestions in to a json
  • transforming it to a eslint format
  • and then run the es-lint fixer?

@ChristianMurphy
Copy link
Member

is there a way to use the eslint ecosystem

Maybe, can you clarify what you mean?

  • exporting remark-lint messages with fixes and suggestions in to a json
  • transforming it to a eslint format
  • and then run the es-lint fixer?

Again, maybe.
Can you explain the value add of this?
The rule syntax of unified currently doesn't have suggested fixes exported.
Either way this would need to be implemented.
It sounds like this is suggesting implementing a fixer, then disabling it, and exporting to ESLint?

@ChristianMurphy
Copy link
Member

Another approach for autofixing, using an attacher with linting context, and a transformer to update the document, similar to Stylelint's approach.
https://stylelint.io/developer-guide/rules#add-autofix

@JounQin
Copy link
Member

JounQin commented Aug 10, 2020

@muescha Actually eslint-mdx is doing this.

@muescha
Copy link

muescha commented Aug 10, 2020

@JounQin i did not found any docs about it... can you point me to it?

@JounQin
Copy link
Member

JounQin commented Aug 11, 2020

@muescha I've pointed the doc link. https://github.com/mdx-js/eslint-mdx#mdxremark

mdx/remark

Integration with remark-lint plugins, it will read remark's configuration automatically via cosmiconfig. But .remarkignore will not be respected, you should use .eslintignore instead.

@muescha
Copy link

muescha commented Aug 13, 2020

possible fixable depends on your remark plugins:

🤔 i did not expected it behind this sentence ...

@coderaiser
Copy link
Contributor

coderaiser commented Nov 14, 2021

Would be amazing to have ability to determine that rule should make some modifications to markdown file or just to report.
For example 🐊 Putout rule has a split to fix, report and traverse:

module.exports.report = () => 'Avoid debugger statement';

module.exports.fix = (path, {options}) => {
    path.remove();
};

module.exports.traverse = ({push}) => ({
    'debugger'(path) {
        push(path);
    },
});

So when you pass --fix it will call the method fix and remove the node, otherwise it will call report.
Right now I have two remark-lint rules:

remove-dependency-status-badge.md:

import {lintRule} from 'unified-lint-rule';

export const removeDependenciesStatusBadge = lintRule('remark-lint:remove-dependencies-status-badge', (tree, file) => {
    const children = tree.children.filter(isDependencyStatus(file));
    tree.children = children;

    const [heading] = children;

    if (heading.type !== 'heading')
        return;

    const headingChildren = heading.children.filter(isDependencyLink(file));
    tree.children[0].children = headingChildren;
});

const isDependencyStatus = (file) => (child) => {
    if (child.type !== 'definition')
        return true;

    if (child.label === 'DependencyStatusURL') {
        file.message('Remove DependencyStatusURL', child);
        return false;
    }

    if (child.label === 'DependencyStatusIMGURL') {
        file.message('Remove DependencyStatusIMGURL', child);
        return false;
    }

    return true;
};

const isDependencyLink = (file) => (child) => {
    if (child.type !== 'linkReference')
        return true;

    if (child.children[0].label === 'DependencyStatusIMGURL') {
        file.message('Remove reference to DependencyStatusIMGURL', child);
        return false;
    }

    return true;
};

And remove-trailing-whitespaces-from-heading:

import {lintRule} from 'unified-lint-rule';

export const removeTrailingWhitespacesFromHeading = lintRule('remark-lint:remove-trailing-whitespaces-from-heading', (tree, file, options) => {
    console.log(options);
    const [heading] = tree.children;

    if (heading.type !== 'heading')
        return;

    const latest = heading.children[heading.children.length - 1];

    if (latest.type === 'text' && / $/.test(latest.value)) {
        latest.value = latest.value.slice(0, -1);
        report(file, latest);
    }

    tree.children[0].children = heading.children;
});

function report(file, child) {
    file.message('Avoid trailing whitespaces', child);
}

I'm using remark-lint in processor-markdown and when I run 🐊 Putout without --fix flag I have information about trailing whitespaces and dependency status badge. But I need only information about dependency badge. But AST modified and everything is reported at once.

Actually there is a possibility to pass fix flag in rule options (this is not quite right, but possible). But I'm using preset according to create custom rule tutorial, and cannot pass any options from the outside.

@wooorm could you please suggest me a way to handle this case and not modify AST in report mode and only do changes in fix mode?

@coderaiser
Copy link
Contributor

coderaiser commented Nov 14, 2021

Here is what I came up with. Create one runner rule:

import {lintRule} from 'unified-lint-rule';
import removeDependenciesStatusBadge from './remove-dependencies-status-badge.mjs';
import removeTrailingWhitespacesFromHeading from './remove-trailing-whitespaces-from-heading.mjs';

const plugins = [
    removeDependenciesStatusBadge,
    removeTrailingWhitespacesFromHeading,
];

const maybeEmptyArray = (a) => isArray(a) ? a : [];

const {isArray} = Array;

export const run = lintRule('remark-lint:run', (tree, file, options) => {
    for (const {fix, traverse, report, name} of plugins) {
        const nodes = traverse(tree);

        for (const node of maybeEmptyArray(nodes)) {
            if (options.fix) {
                fix(node);
                continue;
            }

            const message = report(node);
            file.message(`${name}: ${message}`, node);
        }
    }
});

export const rules = {
    plugins,
};

And here is how my rules looks like:

remove-dependencies-status-badge:

const report = () => 'Remove dependencies status badge';

export default {
    name: 'remove-dependencies-status-badge',
    traverse,
    fix,
    report,
};

function fix(nodes, tree) {
    const children = tree.children.filter(isDependencyStatus(nodes));
    tree.children = children;

    const [heading] = children;

    if (heading.type !== 'heading')
        return;

    const headingChildren = heading.children.filter(isDependencyLink);
    tree.children[0].children = headingChildren;
}

function traverse(tree) {
    const nodes = [];
    tree.children.filter(isDependencyStatus(nodes));
    const [first] = nodes;

    return [first];
}

const isDependencyStatus = (nodes) => (child) => {
    if (child.type !== 'definition')
        return true;

    if (child.label === 'DependencyStatusURL') {
        nodes.push(child);
        return false;
    }

    if (child.label === 'DependencyStatusIMGURL') {
        nodes.push(child);
        return false;
    }

    return true;
};

const isDependencyLink = (child) => {
    if (child.type !== 'linkReference')
        return true;

    if (child.children[0].label === 'DependencyStatusIMGURL')
        return false;

    return true;
};

And remove-trailing-whitespaces-from-heading:

const report = () => 'Avoid trailing whitespaces';

const fix = (heading, tree) => {
    const latest = heading.children[heading.children.length - 1];

    if (latest.type === 'text' && / $/.test(latest.value))
        latest.value = latest.value.slice(0, -1);

    tree.children[0].children = heading.children;
};

const traverse = (tree) => {
    const [heading] = tree.children;

    if (heading.type !== 'heading')
        return;

    const latest = heading.children[heading.children.length - 1];

    if (latest.type === 'text' && / $/.test(latest.value))
        return [latest];
};

export default {
    name: 'remove-trailing-whitespaces-from-heading',
    fix,
    traverse,
    report,
};

And everything works good. But! This format is incompatible with format used by remark-lint, and I need to determine is it fix-format-rule with fix in options, or it's remark-lint rule.

By the way similar format looks amazing for ESLint, here is example. It's much shorter and simpler then most ESLint rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🌊 blocked/upstream This cannot progress before something external happens first help wanted 🙏 This could use your insight or help 👀 no/external This makes more sense somewhere else 🧑 semver/major This is a change 🦋 type/enhancement This is great to have 🙆 yes/confirmed This is confirmed and ready to be worked on
Development

No branches or pull requests

7 participants