-
Notifications
You must be signed in to change notification settings - Fork 6.8k
chore(): forbidden identifiers check for relative scope imports. #764
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
chore(): forbidden identifiers check for relative scope imports. #764
Conversation
@@ -33,7 +34,16 @@ const blocked_statements = [ | |||
'from \\\'rxjs/Rx\\\'', | |||
]; | |||
|
|||
// Retrieves all scope packages dynamically from the source. | |||
// Developers shouldn't be able to import from other scope packages by using relative paths. | |||
const conpmponentFolders = fs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "componentFolders"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good catch - definitely a cool naming convention :)
* Checks the added imports for any relative scope imports and automatically suggests the correct Scope Import statement.
e840185
to
010d436
Compare
@hansl can you review this one? |
const blockedRegex = new RegExp(blocked_statements.join('|'), 'g'); | ||
const importRegex = /from '(.*)'/g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't check if the line is an import
. What if from '...'
is put inside a comment?
There's no cases where you'd have multiple imports on a single line. I suggest the following regex:
const importRegex = /^import.*from\s+'(\S*)';/;
Note that anything else would fail linting, so really this wouldn't be a problem.
LGTM with this note. Please fix and I'll merge. Cheers! |
} from '@angular/core'; | ||
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from '@angular/common'; | ||
forwardRef | ||
} from '../../core/core'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just commited for testing purposes and I already force-pushed to remove it :)
5938be9
to
c3127eb
Compare
c3127eb
to
e9b4da6
Compare
LGTM! Let's wait for the tests to pass and I'll merge. Thanks a lot! |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Once a forbidden import will be found, we will output a super helpful error message (including suggestion / correction)
cc. @jelbourn