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

feat: Add migrate-config CLI tool #26

Merged
merged 17 commits into from
May 30, 2024
Merged

feat: Add migrate-config CLI tool #26

merged 17 commits into from
May 30, 2024

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented May 24, 2024

This pull request adds a new package, @eslint/migrate-config, that is just a CLI tool designed to be run via npx.

This is different than other packages in a couple ways:

  1. Because there is no API exposed, we don't generate type definitions or publish to JSR.
  2. There is no need for Rollup so the entrypoint is in src.

Note: This is intended to be a first-pass of this tool to get people started. I'm not intending for this to be a perfect solution, but I've tested the output on a bunch of open source projects and found that it typically gets the config around 90% of the way there.

@nzakas
Copy link
Member Author

nzakas commented May 24, 2024

Not entirely sure why JSR thinks the directory is dirty. 🤔

package.json Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

mdjermanovic commented May 24, 2024

Perhaps we could temporarily add git status here to see which files are causing the issue in CI.

"test:jsr": "npx jsr@latest publish --dry-run",

"test:jsr": "git status ; npx jsr@latest publish --dry-run ; git status",

@mdjermanovic
Copy link
Member

Seems to be a problem with changing file permissions

diff --git a/packages/migrate-config/src/migrate-config-cli.js b/packages/migrate-config/src/migrate-config-cli.js
old mode 100644
new mode 100755

@mdjermanovic
Copy link
Member

I think what happens here is that npm install adds execute permissions to package.bin files, which is in this case src/migrate-config-cli.js. But the file is stored without them in git:

$ git ls-files -s packages/migrate-config/src/migrate-config-cli.js
100644 846046275ddde8f522b125e1fbccd8d5171b0c56 0       packages/migrate-config/src/migrate-config-cli.js

What might help is to do this:

$ git update-index --chmod=+x packages/migrate-config/src/migrate-config-cli.js

After this, you should see new permissions:

$ git ls-files -s packages/migrate-config/src/migrate-config-cli.js
100755 846046275ddde8f522b125e1fbccd8d5171b0c56 0       packages/migrate-config/src/migrate-config-cli.js

Then, commit the file & push.

@nzakas
Copy link
Member Author

nzakas commented May 27, 2024

Ah, thanks for digging into that. 🙏 Looks like it worked!

@mdjermanovic
Copy link
Member

It looks like the build script crashes because it assumes that every package that starts with @eslint is in this repo, but that's not the case with @eslint/eslintrc.

Comment on lines +12 to +14
"publishConfig": {
"access": "public"
},
Copy link
Member

Choose a reason for hiding this comment

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

its public by default?

Copy link
Member

Choose a reason for hiding this comment

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

@nzakas
Copy link
Member Author

nzakas commented May 28, 2024

Fixed the build script. 👍

@nzakas
Copy link
Member Author

nzakas commented May 29, 2024

Great feedback, thanks! Updated everything and even cleaned up some unnecessary objects that were being output.

? config.ignorePatterns
: [config.ignorePatterns];
const ignorePatternsArray = b.arrayExpression(
ignorePatterns.map(pattern => b.literal(gitignoreToMinimatch(pattern))),
Copy link
Member

Choose a reason for hiding this comment

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

There are some cases where gitignoreToMinimatch doesn't return patterns that are in the current flat config implementation of global ignores equivalent to eslintrc patterns.

For example:

eslintrc (.gitignore) flat config equivalent gitIgnoreToMinimatch
*/a.js */a.js **/*/a.js
dir/** dir/**/* dir/**
dir/ **/dir/ **/dir/**

Here's a function that I believe should correctly convert patterns:

function convert(pattern) {
    const isNegated = pattern.startsWith("!");
    const negatedPrefix = isNegated ? "!" : "";
	const patternToTest = (isNegated ? pattern.slice(1) : pattern).trimEnd();

    // special cases
    if (["", "**", "/**", "**/"].includes(patternToTest)) {
        return `${negatedPrefix}${patternToTest}`;
    }

    const firstIndexOfSlash = patternToTest.indexOf("/");

    const matchEverywherePrefix =
        firstIndexOfSlash < 0 || firstIndexOfSlash === patternToTest.length - 1
            ? "**/"
            : "";

    const patternWithoutLeadingSlash = 
        firstIndexOfSlash === 0
            ? patternToTest.slice(1)
            : patternToTest;

    const matchInsideSuffix = 
        patternToTest.endsWith("/**")
            ? "/*"
            : "";    

    return `${negatedPrefix}${matchEverywherePrefix}${patternWithoutLeadingSlash}${matchInsideSuffix}`;       
}
Tests
const assert = require("node:assert");

describe("convert", () => {
    const test = {
        "**": "**",
        "/**": "/**",
        "**/": "**/",
        "/**/": "**/",

        "": "",
        " ": "",
        "  ": "",
        "a/b ": "a/b",
        "a/b  ": "a/b",

        "*": "**/*",
        "*/": "**/*/",
        "/*": "*",
        "/*/": "*/",
        "a": "**/a",
        "a/": "**/a/",
        "/a": "a",
        "/a/": "a/",
        "**/*": "**/*",
        "*/a": "*/a",
        "*/a/": "*/a/",
        "/*/a": "*/a",
        "/*/a/": "*/a/",
        "a/b": "a/b",
        "a/b/": "a/b/",
        "/a/b": "a/b",
        "/a/b/": "a/b/",
        "a/**/b": "a/**/b",
        "a/**/b/": "a/**/b/",
        "/a/**/b": "a/**/b",
        "/a/**/b/": "a/**/b/",
        "a/b/**": "a/b/**/*",
        "/a/b/**": "a/b/**/*",
        "/a/b/**": "a/b/**/*",
        "a/b/**/": "a/b/**/",
        "/a/b/**/": "a/b/**/",

        "*/a.js": "*/a.js",
        "dir/**": "dir/**/*",
        "dir/": "**/dir/",
        "dir/**/": "dir/**/"


    };

    for (const [oldPattern, newPattern] of Object.entries(test)) {
        test[`!${oldPattern}`] = `!${newPattern}`;
    }

    for (const [oldPattern, newPattern] of Object.entries(test)) {
        it(`for "${oldPattern}" should return ${newPattern}`, () => {
            assert.strictEqual(convert(oldPattern), newPattern);
        });
    }
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you saying this is a bug in gitignoreToMinimatch? Or that we are doing something unique?

Copy link
Member

Choose a reason for hiding this comment

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

Are you saying this is a bug in gitignoreToMinimatch?

Depends on what it is designed for. In earlier iterations of config-array, global ignore patterns were matching only files, and gitignoreToMinimatch indeed translates gitignore patterns that match files and directories to minimatch patterns that are intended to match files (though such translation can't guarantee correctness). But now config-array global ignore patterns can match directories too.

Or that we are doing something unique?

What we now have in config-array is like a more powerful gitignore. Minimatch patterns provide more features than gitignore patterns (for example, this would be much more difficult to achieve with gitignore patterns), and except for the differences in individual patterns, principles are the same as in gitignore:

  • Patterns can match both files and directories.
  • Files in ignored directories are considered ignored.
  • Unignoring works the same.

So it's easy to switch from gitignore reasoning to config-array ignore reasoning because it's generally the same + enhanced syntax.

There are only two notable differences in semantics, which the above code addresses (and users should be aware of):

  • In config-array, patterns are always relative to the root, whereas in gitignore, patterns that don't have a slash at the beginning or in the middle can match at any level.
  • In config-array, foo/** matches directory foo/ in addition to everything inside (foo/** should not match foo/ isaacs/minimatch#179), whereas in gitignore it doesn't match directory foo/, only everything inside.

There would be more work to achieve 100% correctness, e.g., some literal characters in gitignore patterns can have syntactic meaning in minimatch patterns, but those are very edge cases I believe.

@nzakas
Copy link
Member Author

nzakas commented May 30, 2024

Updated the .eslintignore conversion. 👍

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit b16dd8d into main May 30, 2024
14 checks passed
@mdjermanovic mdjermanovic deleted the migrate-config branch May 30, 2024 17:39
@github-actions github-actions bot mentioned this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants