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

add a deprecation_start_version argument to warn_or_error() #7494

Conversation

Projects
None yet
5 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 3, 2019

Problem

--pants-runtime-python-version is something we want to deprecate in 1.17.0.dev0, not right now. Currently we have to rely on TODOs and memory to perform this form of delayed deprecation.

Solution

  • Add a deprecation_start_version option to warn_or_error() in deprecated.py before which a deprecation warning is not shown.

Result

It's possible to schedule deprecations to occur in the future without losing track of TODOs.

@cosmicexplorer cosmicexplorer requested review from stuhood, jsirois and benjyw Apr 3, 2019

@jsirois

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Can this be inverted? IE: let the user determine if they are overwhelmed by a warning and give them a general knob to silence individual ones as needed for as long as needed? Maybe aka thread the existing support for warning filters already present in the stdlib through an option?

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-extended-deprecation-period branch from 10572fa to da5a9ff Apr 4, 2019

@cosmicexplorer cosmicexplorer changed the title add a deprecation start version argument to warn_or_error pipe in a --warning-filters option to filter warnings Apr 4, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Done! 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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

@Eric-Arellano brought up that the original PR which allowed delaying deprecation warnings would be pretty useful for things like pants_runtime_python_version, which is planned to be deprecated at 1.17.0.dev0 -- this isn't something we'd want to fix with warning filters, I believe.

I'll split out the current diff into a new PR and restore the original one here.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-extended-deprecation-period branch from da5a9ff to 10572fa Apr 4, 2019

@cosmicexplorer cosmicexplorer changed the title pipe in a --warning-filters option to filter warnings add a deprecation_start_version argument to warn_or_error Apr 4, 2019

@cosmicexplorer cosmicexplorer changed the title add a deprecation_start_version argument to warn_or_error add a deprecation_start_version argument to warn_or_error() Apr 4, 2019

@benjyw

benjyw approved these changes Apr 4, 2019

Copy link
Contributor

left a comment

Just some comment nits.

Show resolved Hide resolved src/python/pants/base/deprecated.py
Show resolved Hide resolved src/python/pants/option/global_options.py Outdated
Show resolved Hide resolved src/python/pants/option/global_options.py Outdated

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-extended-deprecation-period branch from dfec830 to d48bf64 Apr 4, 2019

@jsirois

jsirois approved these changes Apr 4, 2019

Show resolved Hide resolved src/python/pants/base/deprecated.py Outdated
Show resolved Hide resolved src/python/pants/base/deprecated.py Outdated
@jsirois

jsirois approved these changes Apr 4, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Good use of mock in the tests!

Show resolved Hide resolved src/python/pants/base/deprecated.py
@@ -126,6 +126,16 @@ def register_bootstrap_options(cls, register):
'version of the pants instance you are running using -v, -V, or --version.')

register('--pants-runtime-python-version', advanced=True,
removal_version='1.19.0.dev0',
deprecation_start_version='1.17.0.dev0',
removal_hint="""\

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Apr 4, 2019

Contributor

Preferred to use from textwrap import dedent so that you can ident the below text to be consistent with the rest of the block.

    removal_hint=dedent("""\
      Hello
      New line""")

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 4, 2019

Author Contributor

So the reason I wanted to format it like this is because this string is going to be output to the terminal directly, and I wanted to have it look the way it would when output to the terminal, including having line widths be the same as when output to the terminal. Does that seem to be a good reason to avoid the norm here?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 4, 2019

Author Contributor

I've added an initial newline so that the first line reflects the way it is output to the terminal as well.

Show resolved Hide resolved tests/python/pants_test/test_base.py
Show resolved Hide resolved tests/python/pants_test/test_base.py

cosmicexplorer added some commits Apr 4, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:add-extended-deprecation-period branch from 9e68ab4 to 6aac768 Apr 5, 2019

@Eric-Arellano
Copy link
Contributor

left a comment

Thank you! Looks great.

@cosmicexplorer cosmicexplorer merged commit fd27b4d into pantsbuild:master Apr 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stuhood

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I dig it. Thank you!

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.