Navigation Menu

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

Only emit each warning once per source location #1322

Merged
merged 3 commits into from May 21, 2021
Merged

Only emit each warning once per source location #1322

merged 3 commits into from May 21, 2021

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented May 21, 2021

@nex3 nex3 requested a review from Awjin May 21, 2021 20:40
value is SassString ? value.text : _serialize(value, node.expression);

// Don't use [_warn] because we don't want to pass the span to the logger.
if (_warningsEmitted.add(Tuple2(message, node.span))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should @warn be deduplicated too ?

For warnings triggered in dart code, the span is the place that you actually need to change to resolve the warning (at least in most cases).
For userland functions triggering a warning with @warn when being misused, the span used here would be the location of the @warn rule inside the function, not the location of the caller (which is what where the change would have to be done).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, that's a good point. We don't necessarily have a good understanding of where the boundary is between "user code" and "library code" such that we can reliably tell which stack frame has the span that should actually be used for uniqueness here. I'll drop the @warn de-duplication.

lib/src/visitor/async_evaluate.dart Outdated Show resolved Hide resolved
@nex3 nex3 requested a review from Awjin May 21, 2021 22:25
@nex3 nex3 merged commit 7f982a1 into master May 21, 2021
@nex3 nex3 deleted the warn-once branch May 21, 2021 23:23
@nex3 nex3 mentioned this pull request May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants