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 report abstract-related warnings on warnings=all #8010

Merged
merged 2 commits into from Jul 30, 2019

Conversation

@asterite
Copy link
Member

commented Jul 29, 2019

Follow up to #7999

@asterite asterite requested review from bcardiff and RX14 Jul 29, 2019

@@ -221,6 +221,10 @@ class Crystal::AbstractDefChecker
@program.colorize("The above warning will become an error in a future Crystal version.").yellow.bold
end

private def report_warning(warning)

This comment has been minimized.

Copy link
@bcardiff

bcardiff Jul 30, 2019

Member

We should be filtering by location also. The ignore_warning_due_to_location method has the logic for this.
I think that calling it with base_method.location should do it. Otherwise, the warnings triggered by the dependencies will appear.

Of course, it's a balance and that hides a bit the need for an update in dependencies. Similar to the formatter tool that by default ignore libs, warnings do that as well (yet due to how does flags work the user is unable to include libs in the check...).

So, if you explicitly prefer to not filter by location I am ok for this PR.

This comment has been minimized.

Copy link
@asterite

asterite Jul 30, 2019

Author Member

Oh, I didn't know warnings were ignored for libs. That makes sense. I'll fix it now.

This comment has been minimized.

Copy link
@asterite

asterite Jul 30, 2019

Author Member

Done! Let me know what you think, I moved the ignore_warning_due_to_location to Program so I could reuse it (I also think it makes sense for it to be there). I also added a ? at the end of it and returned an extra return.

Abstract warnings: ignore them according to their location
While on it, move the logic to decide whether a location is ignored or
not to Program.

@bcardiff bcardiff merged commit 489d073 into crystal-lang:master Jul 30, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bcardiff bcardiff added this to the 0.30.0 milestone Jul 30, 2019

@RX14 RX14 added the topic:compiler label Jul 30, 2019

@asterite asterite deleted the asterite:abstract-warning-check-flag branch Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.