-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add new rule: deprecated_member_use_from_same_package #4153
Conversation
addTestFile(code); | ||
await resolveTestFile(); | ||
var filteredErrors = errors | ||
.whereNot((e) => |
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.
I'm curious what errors you're filtering out here? Could we re-write this to be more targeted? (My concern is that we not ignore/mask surprising errors in the test data.) FWIW I'm all for enumerating a handful of diagnostics that might be ignored in a lot of tests. (I was thinking just today how we might want to ignore unused_local_variable
at least.)
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.
These are targeted at the two error codes listed. What are you thinking for "more targeted"? I'm open to ideas here, cuz it's a little weird. I can just add a comment too.
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.
The filter removed everything except the listed codes. I think the question is: what codes are being removed? In general we try to write code that doesn't produce any diagnostics other than the ones we're explicitly testing for. That helps us catch code that needs to be updated as the language changes, but ignoring everything negates that benefit.
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.
I might be thinking about this wrong, but normally we take all reported errors and compare them against the short list of expected errors.
I think the question is: what codes are being removed?
In this bit, I am taking all errors which are not HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE
or HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE_WITH_MESSAGE
. So it's the same set of reported errors as usual, minus these two.
ignoring everything negates that benefit.
I certainly don't intend to ignore everything. Just two error codes.
These two error codes are produced today, but will not be produced soon, with an SDK commit. The only way to get the tests to pass in google3 in both states is to filter out the HintCodes.
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.
I read the code backwards and thought you were filtering out everything except those two. Filtering those two is fine.
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.
Ah, maybe I read it backwards too!
addTestFile(code); | ||
await resolveTestFile(); | ||
var filteredErrors = errors | ||
.whereNot((e) => |
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.
The filter removed everything except the listed codes. I think the question is: what codes are being removed? In general we try to write code that doesn't produce any diagnostics other than the ones we're explicitly testing for. That helps us catch code that needs to be updated as the language changes, but ignoring everything negates that benefit.
Description
Fixes #4118
This rule implements (more or less*) the same feature currently implemented in the analyzer
HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE
.See this comment on the issue for the steps to roll this to analyzer, which involve simultaneously removing
HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE
.*: The changes are improvements to the rule, which are similarly proposed and in-progress for
HintCode.DEPRECATED_MEMBER_USE
: dart-lang/sdk#51664.