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

glob pattern in except option of the no-restricted-paths rule #1708

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

KevinHerklotz
Copy link

Allow to also pass a glob pattern to the except option of the no-restricted-paths rule.
Before you could only add a relative path. This is still working and the new glob pattern is also only working in the relative path.

I also improved the documentation in general.

Fixes #1701

@coveralls
Copy link

coveralls commented Mar 29, 2020

Coverage Status

Coverage increased (+0.01%) to 97.918% when pulling 888443e on KevinHerklotz:add-glob-to-except into ff50964 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.808% when pulling a108a4a on KevinHerklotz:add-glob-to-except into 71ca88f on benmosher:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.808% when pulling a108a4a on KevinHerklotz:add-glob-to-except into 71ca88f on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.808% when pulling a108a4a on KevinHerklotz:add-glob-to-except into 71ca88f on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.808% when pulling a108a4a on KevinHerklotz:add-glob-to-except into 71ca88f on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.808% when pulling a108a4a on KevinHerklotz:add-glob-to-except into 71ca88f on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.808% when pulling a108a4a on KevinHerklotz:add-glob-to-except into 71ca88f on benmosher:master.

@KevinHerklotz

This comment has been minimized.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
```

The following patterns are not considered problems when configuration set to `{ "zones": [ { "target": "./client", "from": "./server" } ] }`:
Copy link
Member

Choose a reason for hiding this comment

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

The "The following patterns" wording follows eslint's own docs; please restore it in both places.

let exceptionGlobs = []

exceptions.forEach(exception => {
if (isGlob(exception)) {
Copy link
Member

Choose a reason for hiding this comment

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

is this not something minimatch can validate?

Copy link
Author

Choose a reason for hiding this comment

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

I just looked into it, but unfortunately minimatch is not enough here. It would just tell me that it does not match, but it doesn't tell me if it is because the glob is not matching or because it cannot handle relative paths. If I don't know that I would also not know when to call reportInvalidExceptionPath.
So I guess we have to use isGlob here.

Copy link
Member

Choose a reason for hiding this comment

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

what do you mean by "cannot handle relative paths"? is there a need for a different error message between globs and paths?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, maybe I wasn't clear enough.
isGlob checks if it is a glob and thus a relative path like "./server" will be marked as non-glob ("exceptionPaths"). Otherwise if I would check for example "./server" with minimatch it will tell me this path doesn't match, no matter if this path would actually match, because minimatch only works with globs.

Also the check if you have an invalid exception path is done by path.relative(), which also does not work with globs. That's why you will currently not see the "Restricted path exceptions must be descendants of the configured from path for that zone." error message for any glob you provide.
And I think it is totally ok, because you would use globs for different reasons anyways. For example if you want to exclude all "**/types.js", you cannot really check if this is a descendant (you could check if there is any file that matches, but this would take a lot of resources and wouldn't make sense to me).

@KevinHerklotz
Copy link
Author

To be honest I'm a bit lost now and don't really understand why the tests are failing. The error messages are not really helpful for me. Help from someone else is much appreciated.

@ljharb
Copy link
Member

ljharb commented Aug 8, 2021

@KevinHerklotz sorry for the delay. i've partially rebased this, and tests seem to be passing locally :-) but it'll need another rebase onto latest master.

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

Successfully merging this pull request may close these issues.

no-restricted-paths improve except
3 participants