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

[New] no-restricted-paths: support arrays for from and target options #2466

Merged

Conversation

AdriAt360
Copy link
Contributor

@AdriAt360 AdriAt360 commented Jun 1, 2022

Fixes #2142

Context

Sometimes I want to set certain paths in batch to prohibit relying on the same path, or a certain path to prohibit relying on multiple paths.

Changes

In this PR

  • I update the schema of this rule
    • before: from and target are only a string
    • after: these fields can be either a string or an array of string
  • I change the behavior + add tests
    • all the elements in from and target are taken into account the same way it was before
    • I also ensure the homogeneity of the from elements to stay aligned with the current homogeneity checks between from and except
  • I update the related doc and add new examples
  • I update the changelog

Notes

  • I advise to read this PR commit by commit as there are quite a few changes that are easier to review step by step, e.g. extracting functions before using them inside loops
  • I chose to support both string and array to avoid a breaking change

docs/rules/no-restricted-paths.md Outdated Show resolved Hide resolved
src/rules/no-restricted-paths.js Outdated Show resolved Hide resolved
src/rules/no-restricted-paths.js Outdated Show resolved Hide resolved
src/rules/no-restricted-paths.js Show resolved Hide resolved
src/rules/no-restricted-paths.js Outdated Show resolved Hide resolved
tests/src/rules/no-restricted-paths.js Show resolved Hide resolved
tests/src/rules/no-restricted-paths.js Outdated Show resolved Hide resolved
src/rules/no-restricted-paths.js Outdated Show resolved Hide resolved
src/rules/no-restricted-paths.js Outdated Show resolved Hide resolved
@AdriAt360 AdriAt360 requested a review from ljharb June 2, 2022 11:30
@ljharb ljharb force-pushed the issue-2142-no-restricted-paths-arrays branch from 6b4ea9f to 16324aa Compare June 3, 2022 18:27
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

I'll make these changes in a rebase. Thanks!

src/rules/no-restricted-paths.js Show resolved Hide resolved
src/rules/no-restricted-paths.js Show resolved Hide resolved
src/rules/no-restricted-paths.js Outdated Show resolved Hide resolved
@AdriAt360
Copy link
Contributor Author

@ljharb my pleasure!
thanks for your review and suggestions 🙏🏽
how do you want to proceed for the next steps: as I have no write access, I guess you will merge this pull request yourself and close the related issue when done?

@ljharb
Copy link
Member

ljharb commented Jun 3, 2022

Yes, I'll merge it myself, and the fixes #2142 i edited into your OP will cause github to auto-close it upon merging.

@AdriAt360
Copy link
Contributor Author

@ljharb indeed, I missed your edit, all perfect then =)

@ljharb ljharb changed the title feat(2142): no-restricted-paths - add support for arrays in from and target fields [New] no-restricted-paths: support arrays for from and target options Jun 3, 2022
@ljharb
Copy link
Member

ljharb commented Jun 3, 2022

@AdriAt360 hm, looks like this PR is causing the native windows tests to fail: https://ci.appveyor.com/project/ljharb/eslint-plugin-import/build/job/xqkwnmajs2svu218

@AdriAt360
Copy link
Contributor Author

AdriAt360 commented Jun 3, 2022

@ljharb as there are multiple tests with this name and I can't reproduce locally, I just added some comments to identify the failing test d3fa030
I hope it will help investigating and will keep you posted

@ljharb
Copy link
Member

ljharb commented Jun 3, 2022

looks like number 7: https://ci.appveyor.com/project/ljharb/eslint-plugin-import/builds/43753304/job/dshd09a4o6tucs0a

@AdriAt360
Copy link
Contributor Author

AdriAt360 commented Jun 7, 2022

@ljharb still no way to reproduce on my side, so I added another test to help me investigate

@AdriAt360
Copy link
Contributor Author

@ljharb ok, seems that this specific glob pattern (./some/path/*) is not accepted by windows
I updated and added some tests to check some other common patterns

@AdriAt360
Copy link
Contributor Author

AdriAt360 commented Jun 8, 2022

@ljharb I guess I found the issue, but as it impacts all the tests, you might want to give an opinion
as stated in https://stackoverflow.com/a/39111164/2814072, path.resolve always resolves to an absolute path where path.join might or not
problem is:

  • in test/utils, the testFilePath function used path.join, but in the source code of the no-restricted-paths rule, only path.resolve is used
  • in case of glob patterns, minimatch will match the file only if the file path is absolute, as the glob pattern is
    image
  • I guess on Windows, path.join resolved to a relative path, explaining the failing tests

If this hypothesis is correct, the pipeline should be all green now, so the next step would be to clean the tests I added to investigate here, unless you prefer keeping them

@AdriAt360
Copy link
Contributor Author

AdriAt360 commented Jun 8, 2022

@ljharb ok, wrong hypothesis
another hint could be process.cwd()

More details here
  • from https://www.geeksforgeeks.org/node-js-process-cwd-method/, I can see that the current working directory resolves with back-slashes
    image
  • from https://www.npmjs.com/package/minimatch, I can see that the glob pattern should contain only forward-slashes
    image
  • but in our situation
    • target is defined correctly (only /)
    • if no basePath is provided, process.cwd() is used as basePath to build targetPath
    • meaning on Windows native, the target path pattern could resolve with \ in some conditions (I guess, don't know why it could not be everytime)
      I opened this other PR to validate this hypothesis (expected output: the new test I added there is red too), I'll close it

As it seems be working with the from and except parts, I change to use Minimatch.match as it's done there, it should fix this for real: 4392f46

(by the way I reverted the previous commit too)

if this is really the root cause, then I guess there should be a dedicated [Fix] commit for this problem, as you already did for the error message, so I can clean and force-push here

@ljharb ljharb force-pushed the issue-2142-no-restricted-paths-arrays branch from 4392f46 to 55621d8 Compare June 10, 2022 20:52
@AdriAt360
Copy link
Contributor Author

AdriAt360 commented Jun 16, 2022

hi there @ljharb 👋🏽
as my previous attempt wasn't successful either, here is another try to fix the existing issue on the Windows Native pipeline

I hope this will make the pipeline green 🤞🏽

edit: seems that it doesn't work either ><

@AdriAt360
Copy link
Contributor Author

@ljharb how do you want to proceed next here?

  • as it was already broken (confirmed by [Tests] no-restricted-paths: skip failing tests on windows #2472), I think it would not be more harmful to merge this current version (without my last 2 commits obviously)
  • in the meantime, I'm not fond of ignoring a broken pipeline
  • but I must admit I run out of new ideas for this windows native pipeline problem

@ljharb
Copy link
Member

ljharb commented Jun 16, 2022

hmm. let's reopen #2472, and update it so that it skips the broken tests in windows. then we can land that, rebase this, and land it.

@AdriAt360 AdriAt360 force-pushed the issue-2142-no-restricted-paths-arrays branch from 2f9ba90 to adaadae Compare June 21, 2022 09:25
@AdriAt360
Copy link
Contributor Author

@ljharb I just commented the failing test and removed my previous investigation commits -> this PR seems ready to be merged
the AppVeyor build failed but it seems like a timeout on a random test outside this PR's scope: could you rerun this check?
By the way, feel free to rebase if you feel the 2 commits 786b9b3 and 1196129 should be merged into only one

@ljharb ljharb force-pushed the issue-2142-no-restricted-paths-arrays branch from adaadae to f6624b8 Compare June 22, 2022 19:45
@ljharb ljharb force-pushed the issue-2142-no-restricted-paths-arrays branch from f6624b8 to 43058f6 Compare June 22, 2022 21:01
@AdriAt360
Copy link
Contributor Author

@ljharb it seems that this PR is ready to be merged, but feel free to tell if there is still something waiting on what I could be helpful :)

@ljharb ljharb force-pushed the issue-2142-no-restricted-paths-arrays branch from 43058f6 to d1fe8eb Compare June 29, 2022 15:51
@ljharb ljharb merged commit d1fe8eb into import-js:main Jun 29, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jan 13, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint-plugin-import](https://github.com/import-js/eslint-plugin-import) | devDependencies | minor | [`2.26.0` -> `2.27.4`](https://renovatebot.com/diffs/npm/eslint-plugin-import/2.26.0/2.27.4) |

---

### Release Notes

<details>
<summary>import-js/eslint-plugin-import</summary>

### [`v2.27.4`](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#&#8203;2274---2023-01-11)

[Compare Source](import-js/eslint-plugin-import@v2.27.3...v2.27.4)

##### Fixed

-   `semver` should be a prod dep (\[[#&#8203;2668](import-js/eslint-plugin-import#2668)])

### [`v2.27.3`](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#&#8203;2273---2023-01-11)

[Compare Source](import-js/eslint-plugin-import@v2.27.2...v2.27.3)

##### Fixed

-   \[`no-empty-named-blocks`]: rewrite rule to only check import declarations (\[[#&#8203;2666](import-js/eslint-plugin-import#2666)])

### [`v2.27.2`](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#&#8203;2272---2023-01-11)

[Compare Source](import-js/eslint-plugin-import@v2.27.1...v2.27.2)

##### Fixed

-   \[`no-duplicates`]: do not unconditionally require `typescript` (\[[#&#8203;2665](import-js/eslint-plugin-import#2665)])

### [`v2.27.1`](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#&#8203;2271---2023-01-11)

[Compare Source](import-js/eslint-plugin-import@v2.27.0...v2.27.1)

##### Fixed

-   `array.prototype.flatmap` should be a prod dep (\[[#&#8203;2664](import-js/eslint-plugin-import#2664)], thanks \[[@&#8203;cristobal](https://github.com/cristobal)])

### [`v2.27.0`](https://github.com/import-js/eslint-plugin-import/blob/HEAD/CHANGELOG.md#&#8203;2270---2023-01-11)

[Compare Source](import-js/eslint-plugin-import@v2.26.0...v2.27.0)

##### Added

-   \[`newline-after-import`]: add `considerComments` option (\[[#&#8203;2399](import-js/eslint-plugin-import#2399)], thanks \[[@&#8203;pri1311](https://github.com/pri1311)])
-   \[`no-cycle`]: add `allowUnsafeDynamicCyclicDependency` option (\[[#&#8203;2387](import-js/eslint-plugin-import#2387)], thanks \[[@&#8203;GerkinDev](https://github.com/GerkinDev)])
-   \[`no-restricted-paths`]: support arrays for `from` and `target` options (\[[#&#8203;2466](import-js/eslint-plugin-import#2466)], thanks \[[@&#8203;AdriAt360](https://github.com/AdriAt360)])
-   \[`no-anonymous-default-export`]: add `allowNew` option (\[[#&#8203;2505](import-js/eslint-plugin-import#2505)], thanks \[[@&#8203;DamienCassou](https://github.com/DamienCassou)])
-   \[`order`]: Add `distinctGroup` option (\[[#&#8203;2395](import-js/eslint-plugin-import#2395)], thanks \[[@&#8203;hyperupcall](https://github.com/hyperupcall)])
-   \[`no-extraneous-dependencies`]: Add `includeInternal` option (\[[#&#8203;2541](import-js/eslint-plugin-import#2541)], thanks \[[@&#8203;bdwain](https://github.com/bdwain)])
-   \[`no-extraneous-dependencies`]: Add `includeTypes` option (\[[#&#8203;2543](import-js/eslint-plugin-import#2543)], thanks \[[@&#8203;bdwain](https://github.com/bdwain)])
-   \[`order`]: new `alphabetize.orderImportKind` option to sort imports with same path based on their kind (`type`, `typeof`) (\[[#&#8203;2544](import-js/eslint-plugin-import#2544)], thanks \[[@&#8203;stropho](https://github.com/stropho)])
-   \[`consistent-type-specifier-style`]: add rule (\[[#&#8203;2473](import-js/eslint-plugin-import#2473)], thanks \[[@&#8203;bradzacher](https://github.com/bradzacher)])
-   Add \[`no-empty-named-blocks`] rule (\[[#&#8203;2568](import-js/eslint-plugin-import#2568)], thanks \[[@&#8203;guilhermelimak](https://github.com/guilhermelimak)])
-   \[`prefer-default-export`]: add "target" option (\[[#&#8203;2602](import-js/eslint-plugin-import#2602)], thanks \[[@&#8203;azyzz228](https://github.com/azyzz228)])
-   \[`no-absolute-path`]: add fixer (\[[#&#8203;2613](import-js/eslint-plugin-import#2613)], thanks \[[@&#8203;adipascu](https://github.com/adipascu)])
-   \[`no-duplicates`]: support inline type import with `inlineTypeImport` option (\[[#&#8203;2475](import-js/eslint-plugin-import#2475)], thanks \[[@&#8203;snewcomer](https://github.com/snewcomer)])

##### Fixed

-   \[`order`]: move nested imports closer to main import entry (\[[#&#8203;2396](import-js/eslint-plugin-import#2396)], thanks \[[@&#8203;pri1311](https://github.com/pri1311)])
-   \[`no-restricted-paths`]: fix an error message (\[[#&#8203;2466](import-js/eslint-plugin-import#2466)], thanks \[[@&#8203;AdriAt360](https://github.com/AdriAt360)])
-   \[`no-restricted-paths`]: use `Minimatch.match` instead of `minimatch` to comply with Windows Native paths (\[[#&#8203;2466](import-js/eslint-plugin-import#2466)], thanks \[[@&#8203;AdriAt360](https://github.com/AdriAt360)])
-   \[`order`]: require with member expression could not be fixed if alphabetize.order was used (\[[#&#8203;2490](import-js/eslint-plugin-import#2490)], thanks \[[@&#8203;msvab](https://github.com/msvab)])
-   \[`order`]: leave more space in rankings for consecutive path groups (\[[#&#8203;2506](import-js/eslint-plugin-import#2506)], thanks \[[@&#8203;Pearce-Ropion](https://github.com/Pearce-Ropion)])
-   \[`no-cycle`]: add ExportNamedDeclaration statements to dependencies (\[[#&#8203;2511](import-js/eslint-plugin-import#2511)], thanks \[[@&#8203;BenoitZugmeyer](https://github.com/BenoitZugmeyer)])
-   \[`dynamic-import-chunkname`]: prevent false report on a valid webpack magic comment (\[[#&#8203;2330](import-js/eslint-plugin-import#2330)], thanks \[[@&#8203;mhmadhamster](https://github.com/mhmadhamster)])
-   \[`export`]: do not error on TS export overloads (\[[#&#8203;1590](import-js/eslint-plugin-import#1590)], thanks \[[@&#8203;ljharb](https://github.com/ljharb)])
-   \[`no-unresolved`], \[`extensions`]: ignore type only exports (\[[#&#8203;2436](import-js/eslint-plugin-import#2436)], thanks \[[@&#8203;Lukas-Kullmann](https://github.com/Lukas-Kullmann)])
-   `ExportMap`: add missing param to function (\[[#&#8203;2589](import-js/eslint-plugin-import#2589)], thanks \[[@&#8203;Fdawgs](https://github.com/Fdawgs)])
-   \[`no-unused-modules`]: `checkPkgFieldObject` filters boolean fields from checks (\[[#&#8203;2598](import-js/eslint-plugin-import#2598)], thanks \[[@&#8203;mpint](https://github.com/mpint)])
-   \[`no-cycle`]: accept Flow `typeof` imports, just like `type` (\[[#&#8203;2608](import-js/eslint-plugin-import#2608)], thanks \[[@&#8203;gnprice](https://github.com/gnprice)])
-   \[`no-import-module-exports`]: avoid a false positive for import variables (\[[#&#8203;2315](import-js/eslint-plugin-import#2315)], thanks \[[@&#8203;BarryThePenguin](https://github.com/BarryThePenguin)])

##### Changed

-   \[Tests] \[`named`]: Run all TypeScript test (\[[#&#8203;2427](import-js/eslint-plugin-import#2427)], thanks \[[@&#8203;ProdigySim](https://github.com/ProdigySim)])
-   \[readme] note use of typescript in readme `import/extensions` section (\[[#&#8203;2440](import-js/eslint-plugin-import#2440)], thanks \[[@&#8203;OutdatedVersion](https://github.com/OutdatedVersion)])
-   \[Docs] \[`order`]: use correct default value (\[[#&#8203;2392](import-js/eslint-plugin-import#2392)], thanks \[[@&#8203;hyperupcall](https://github.com/hyperupcall)])
-   \[meta] replace git.io link in comments with the original URL (\[[#&#8203;2444](import-js/eslint-plugin-import#2444)], thanks \[[@&#8203;liby](https://github.com/liby)])
-   \[Docs] remove global install in readme (\[[#&#8203;2412](import-js/eslint-plugin-import#2412)], thanks \[[@&#8203;aladdin-add](https://github.com/aladdin-add)])
-   \[readme] clarify `eslint-import-resolver-typescript` usage (\[[#&#8203;2503](import-js/eslint-plugin-import#2503)], thanks \[[@&#8203;JounQin](https://github.com/JounQin)])
-   \[Refactor] \[`no-cycle`]: Add per-run caching of traversed paths (\[[#&#8203;2419](import-js/eslint-plugin-import#2419)], thanks \[[@&#8203;nokel81](https://github.com/nokel81)])
-   \[Performance] `ExportMap`: add caching after parsing for an ambiguous module (\[[#&#8203;2531](import-js/eslint-plugin-import#2531)], thanks \[[@&#8203;stenin-nikita](https://github.com/stenin-nikita)])
-   \[Docs] \[`no-useless-path-segments`]: fix paths (\[[#&#8203;2424](import-js/eslint-plugin-import#2424)], thanks \[[@&#8203;s-h-a-d-o-w](https://github.com/s-h-a-d-o-w)])
-   \[Tests] \[`no-cycle`]: add passing test cases (\[[#&#8203;2438](import-js/eslint-plugin-import#2438)], thanks \[[@&#8203;georeith](https://github.com/georeith)])
-   \[Refactor] \[`no-extraneous-dependencies`] improve performance using cache (\[[#&#8203;2374](import-js/eslint-plugin-import#2374)], thanks \[[@&#8203;meowtec](https://github.com/meowtec)])
-   \[meta] `CONTRIBUTING.md`: mention inactive PRs (\[[#&#8203;2546](import-js/eslint-plugin-import#2546)], thanks \[[@&#8203;stropho](https://github.com/stropho)])
-   \[readme] make json for setting groups multiline (\[[#&#8203;2570](import-js/eslint-plugin-import#2570)], thanks \[[@&#8203;bertyhell](https://github.com/bertyhell)])
-   \[Tests] \[`no-restricted-paths`]: Tests for `import type` statements (\[[#&#8203;2459](import-js/eslint-plugin-import#2459)], thanks \[[@&#8203;golergka](https://github.com/golergka)])
-   \[Tests] \[`no-restricted-paths`]: fix one failing `import type` test case, submitted by \[[@&#8203;golergka](https://github.com/golergka)], thanks \[[@&#8203;azyzz228](https://github.com/azyzz228)]
-   \[Docs] automate docs with eslint-doc-generator (\[[#&#8203;2582](import-js/eslint-plugin-import#2582)], thanks \[[@&#8203;bmish](https://github.com/bmish)])
-   \[readme] Increase clarity around typescript configuration (\[[#&#8203;2588](import-js/eslint-plugin-import#2588)], thanks \[[@&#8203;Nfinished](https://github.com/Nfinished)])
-   \[Docs] update `eslint-doc-generator` to v1.0.0 (\[[#&#8203;2605](import-js/eslint-plugin-import#2605)], thanks \[[@&#8203;bmish](https://github.com/bmish)])
-   \[Perf] \[`no-cycle`], \[`no-internal-modules`], \[`no-restricted-paths`]: use `anyOf` instead of `oneOf` (thanks \[[@&#8203;ljharb](https://github.com/ljharb)], \[[@&#8203;remcohaszing](https://github.com/remcohaszing)])

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC45Ny41IiwidXBkYXRlZEluVmVyIjoiMzQuMTAwLjEifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1724
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[import/no-restricted-paths] configuration items target and form should support array format
2 participants