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

Fix `py-thrift-namespace-clash-check` type issue when logging with `--no-strict` mode #7864

Merged
merged 4 commits into from Jun 6, 2019

Conversation

Projects
None yet
2 participants
@Eric-Arellano
Copy link
Contributor

commented Jun 6, 2019

Problem

When running py-thrift-namespace-clash-check in Twitter's sandbox with --no-strict mode, Pants crashed with the exception:

zip_longest argument #1 must support iteration

This is because we were trying to log a NamespaceExtractionError, and log() only understands Union[str, Tuple[str, str]].

Solution

Fix the issue by using str() to get the underlying message.

Also add an assertion to make sure that we don't introduce this issue again. Once we have MyPy set up, we can remove this assertion.

@Eric-Arellano Eric-Arellano added this to the 1.16.x milestone Jun 6, 2019

@Eric-Arellano Eric-Arellano changed the title Fix `py-thrift-namespace-clash-check` logging issue when using `--no-strict` Fix `py-thrift-namespace-clash-check` type issue when logging with `--no-strict` mode Jun 6, 2019

@stuhood

stuhood approved these changes Jun 6, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Some flakes, but merging.

@stuhood stuhood merged commit 2ebe6ec into pantsbuild:master Jun 6, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

@stuhood stuhood modified the milestones: 1.16.x, 1.17.x Jun 6, 2019

stuhood added a commit that referenced this pull request Jun 6, 2019

Fix `py-thrift-namespace-clash-check` type issue when logging with `-…
…-no-strict` mode (#7864)

### Problem
When running `py-thrift-namespace-clash-check` in Twitter's sandbox with `--no-strict` mode, Pants crashed with the exception:

```
zip_longest argument #1 must support iteration
```

This is because we were trying to log a `NamespaceExtractionError`, and `log()` only understands `Union[str, Tuple[str, str]]`.

### Solution
Fix the issue by using `str()` to get the underlying message.

Also add an assertion to make sure that we don't introduce this issue again. Once we have MyPy set up, we can remove this assertion.

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:fix-thrift-logging-issue branch Jun 6, 2019

Eric-Arellano added a commit that referenced this pull request Jun 6, 2019

Fix `py-thrift-namespace-clash-check` type issue when logging with `-…
…-no-strict` mode (#7864)

### Problem
When running `py-thrift-namespace-clash-check` in Twitter's sandbox with `--no-strict` mode, Pants crashed with the exception:

```
zip_longest argument #1 must support iteration
```

This is because we were trying to log a `NamespaceExtractionError`, and `log()` only understands `Union[str, Tuple[str, str]]`.

### Solution
Fix the issue by using `str()` to get the underlying message.

Also add an assertion to make sure that we don't introduce this issue again. Once we have MyPy set up, we can remove this assertion.

@Eric-Arellano Eric-Arellano removed this from the 1.17.x milestone Jun 6, 2019

stuhood added a commit that referenced this pull request Jun 7, 2019

Fix `py-thrift-namespace-clash-check` type issue when logging with `-…
…-no-strict` mode (#7864)

### Problem
When running `py-thrift-namespace-clash-check` in Twitter's sandbox with `--no-strict` mode, Pants crashed with the exception:

```
zip_longest argument #1 must support iteration
```

This is because we were trying to log a `NamespaceExtractionError`, and `log()` only understands `Union[str, Tuple[str, str]]`.

### Solution
Fix the issue by using `str()` to get the underlying message.

Also add an assertion to make sure that we don't introduce this issue again. Once we have MyPy set up, we can remove this assertion.
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.