feat(boundaries): add ignore directive #9938
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
NicholasLYang
force-pushed
the
nicholasyang/boundaries-ignore-comment
branch
from
d69124b to
8d46bba
Compare
| fn is_ignored_import(comments: &SingleThreadedComments, span: Span) -> bool { | ||
| if let Some(import_comments) = comments.get_leading(span.lo()) { | ||
| for comment in import_comments { | ||
| if comment.text.trim() == "@boundaries-ignore" { |
There was a problem hiding this comment.
What do you think about branding the ignore, and requiring a reason?
// @turbo-boundaries-ignore -- we're yolorepo-ing
A little eslinty, but I think requiring reasons here would be very helpful for maintainers
There was a problem hiding this comment.
Sure. Do you think we should do @turbo-boundaries-ignore? I kinda like just @boundaries-ignore
| // If the import is prefixed with `@boundaries-ignore`, we ignore it, but print | ||
| // a warning | ||
| if Self::is_ignored_import(&comments, *span) { | ||
| if warning_count <= MAX_IGNORE_WARNINGS { |
There was a problem hiding this comment.
Can we add another case here that outputs the total count of warnings that exceeded the max?
| for (import, span, import_type) in finder.imports() { | ||
| // If the import is prefixed with `@boundaries-ignore`, we ignore it, but print | ||
| // a warning | ||
| if Self::is_ignored_import(&comments, *span) { |
There was a problem hiding this comment.
Can we avoid having the display logic inside of check_package and instead have check_package return diagnostics and then have a some sort of "reporter" decide on what to do about each diagnostic? I think we'll want to eventually offer a way to disallow this escape hatch (or on CI display all warnings). Should also make it easier to write unit tests for this codepath.
There was a problem hiding this comment.
Changed it to store the warnings. We could keep the diagnostics and annotate them as disabled, but I think this works fine for now. If we want to disallow the ignore annotation, we can also just make the is_ignored_import check return false
| @@ -5,3 +5,20 @@ import { Ship } from "ship"; | |||
| import { Ship } from "@types/ship"; | |||
| // Import package that is not specified | |||
| import { walkThePlank } from "module-package"; | |||
|
|
|||
| // Import from a package that is not specified, but we have `@boundaries-ignore` on it | |||
| // @boundaries-ignore | |||
There was a problem hiding this comment.
Should this have produced a change in a test fixture where a warning is printed?
There was a problem hiding this comment.
We don't snapshot warnings since they go to stderr. I'm not sure if we should, since that could be pretty brittle (and also makes debugging tests slightly annoying)
NicholasLYang
force-pushed
the
nicholasyang/boundaries-ignore-comment
branch
from
8d46bba to
284dedf
Compare
### Description Add ability to ignore imports for boundaries by commenting `// @boundaries-ignore` ### Testing Instructions Added tests to boundaries fixture
Description
Add ability to ignore imports for boundaries by commenting
// @boundaries-ignoreTesting Instructions
Added tests to boundaries fixture