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

make deprecations respect warnings filters and add --ignore-pants-warnings option #7496

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 4, 2019

Problem

#7493 solved its problem by converting an error to a warning, but it's not likely that a large monorepo is going to be able to solve that particular issue anytime soon. So while we would like to be able to deprecate the --strict option in that PR, we don't think it's reasonable to have a deprecation warning showing for the strict option immediately. But not every pants user is a large monorepo, so we would like to be able to do the right upstream thing (add a deprecation warning) while allowing downstream users to defer showing specific warnings in a structured way.

Solution

  • Add an --ignore-pants-warnings global bootstrap option, which calls warnings.filterwarnings() to filter warning messages by regex matching.
  • Make warn_or_error() in deprecated.py use warnings.warn_explicit(), because it turns out that warnings.showwarning() doesn't respect the warning filters at all (oops!).

Result

It's possible to filter out specific warnings with an option, and it's possible to do that to deprecation warnings now as well.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

As noted in #7494 (comment):

I however haven't added testing as it wasn't clear how. I can see how to e.g. add a task in our fake pants-plugins/ to raise a deprecation warning and then run an integration test, but it's not clear if there's a much simpler way to do that which avoids this hefty plugin harness.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-warning-filters-bootstrap-option branch from da5a9ff to bd36422 Apr 4, 2019

Show resolved Hide resolved src/python/pants/option/global_options.py Outdated
@@ -100,7 +100,7 @@ def _get_frame_info(stacklevel, context=1):


def warn_or_error(removal_version, deprecated_entity_description, hint=None, stacklevel=3,
frame_info=None, context=1, ensure_stderr=False):
frame_info=None, ensure_stderr=False):

This comment has been minimized.

Copy link
@jsirois

jsirois Apr 4, 2019

Member

The context arg is used by prod code.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 4, 2019

Author Contributor

I was able to figure out how to resolve this by monkey patching warnings.showwarning in 47b0044. warn_explicit() just doesn't provide the line argument (see https://github.com/python/cpython/blob/fd83a823a6f268dc97ee2bf7d8a1a88d948446e5/Lib/warnings.py#L300) -- so I was able to retain the current behavior while now respecting warnings filters by patching it within a contextmanager.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-warning-filters-bootstrap-option branch 5 times, most recently from 9dd2dd2 to 901a991 Apr 5, 2019

@cosmicexplorer cosmicexplorer changed the title add --warning-filters option to regex match warning messages add --ignore-pants-warnings option to filter out warning messages Apr 6, 2019

@cosmicexplorer cosmicexplorer changed the title add --ignore-pants-warnings option to filter out warning messages make deprecations use warnings filters and add --ignore-pants-warnings option Apr 6, 2019

@cosmicexplorer cosmicexplorer changed the title make deprecations use warnings filters and add --ignore-pants-warnings option make deprecations respect warnings filters and add --ignore-pants-warnings option Apr 6, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

I couldn't figure out how to do this more cleanly, so I've added an integration test which calls a stub task that just raises a deprecation warning and exits to test the --ignore-pants-warnings option and to test that deprecation warnings respect warnings filters in general (the two key points of this change).

cosmicexplorer added some commits Apr 4, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-warning-filters-bootstrap-option branch from f21ed70 to 548602c Apr 12, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-warning-filters-bootstrap-option branch from 548602c to aa0aa4f Apr 12, 2019

@cosmicexplorer cosmicexplorer merged commit ef33b62 into pantsbuild:master Apr 12, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.