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

convert --skip to --strict in py thrift namespace clash check task #7493

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

commented Apr 3, 2019

Problem

#7345 solves its problem, but introduces another one when trying to use it on a large monorepo which generates lots of python thrift targets. Namely, not all thrift sources will have namespace py, and many thrift sources will declare conflicting python namespaces, since this issue was not previously known. This should be resolved, but doing it all at once is difficult and error-prone.

Solution

  • Convert --skip to --strict in the python thrift source namespace clash check task.
    • Default to non-strict, but also raise a deprecation warning explaining that this task will soon become required.
  • Collect all the namespace extraction errors (for thrift sources which are used in a python_thrift_library(), but don't have a namespace py declaration) and dump them into a file instead of displaying them directly.
    • This is to avoid an overwhelming amount of output for users who may not need to care about this issue.

Result

It is easier to introduce the namespace clash check task into a large monorepo and make fixes incrementally.

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

@stuhood

stuhood approved these changes Apr 3, 2019

def execute(self):
deprecated_conditional(
lambda: not self.get_options().strict,
'1.18.0.dev0',

This comment has been minimized.

Copy link
@stuhood

stuhood Apr 3, 2019

Member

This will trigger a warning immediately for all users (in addition to the other new rendered warning).. I don't think we're quite ready for that internally.

Should probably leave a TODO to consider deprecating the flag in a few versions instead?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 3, 2019

Author Contributor

Hm, ok. I might see if I can canonicalize that by extending our current deprecation methods to support a period before the deprecation warning is shown.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Apr 4, 2019

Author Contributor

Separately, I made #7494.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Merging this to unblock the use of the namespace clash checking in said monorepo.

@cosmicexplorer cosmicexplorer merged commit 4d9ff11 into pantsbuild:master Apr 4, 2019

1 check passed

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

cosmicexplorer added a commit that referenced this pull request Apr 12, 2019

make deprecations respect warnings filters and add --ignore-pants-war…
…nings option (#7496)

### 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()`](https://docs.python.org/2/library/warnings.html#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.
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.