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

Python 3 fixes - fix netrc.py, retry.py, and test_objects.py #6235

Merged
merged 4 commits into from Jul 26, 2018

Conversation

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

Eric-Arellano commented Jul 25, 2018

No description provided.

@@ -30,4 +30,5 @@ def retry_on_exception(func, max_retries, exception_types, backoff_func=lambda n
return func()
except exception_types as e:
logger.debug('encountered exception on retry #{}: {!r}'.format(i, e))
raise
if i == max_retries - 1:
raise e

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 25, 2018

Contributor

That stray raise is meant to raise the last exception. It worked on Py2, but not Py3.

This fix captures the same logic in a way that works on both versions.

"Type names and field names must be valid identifiers: \"<class 'int'>\""
if PY3 else
"Type names and field names can only contain alphanumeric characters and underscores: \"<type 'int'>\""
)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 25, 2018

Contributor

A couple of error messages changed their wording between Py2 and Py3. The changes come from TypeError, and are not hardcoded on our end.

This should not impact any actual source code. The errors still raise in the same situations and the messages are essentially the same.

@cosmicexplorer and I discussed via Slack.

@@ -15,46 +15,46 @@ class RetryTest(unittest.TestCase):
@mock.patch('time.sleep', autospec=True, spec_set=True)
def test_retry_on_exception(self, mock_sleep):
broken_func = mock.Mock()
broken_func.side_effect = IOError('broken')
broken_func.side_effect = OSError('broken')

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 25, 2018

Contributor

Python 3.3 consolidated multiple OS related errors into just OSError. It will still work to have it as IOError, but is confusing in logs because Py3 converts IOError -> OSError behind-the-scenes.

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -30,4 +30,5 @@ def retry_on_exception(func, max_retries, exception_types, backoff_func=lambda n
return func()
except exception_types as e:
logger.debug('encountered exception on retry #{}: {!r}'.format(i, e))
raise
if i == max_retries - 1:
raise e

This comment has been minimized.

@stuhood

stuhood Jul 25, 2018

Member

Should this be raise e or just raise? My understanding was the latter preserves the original stacktrace, which we would want here.

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Jul 25, 2018

Contributor

You're right! Thanks, fixed with newest commit.

@stuhood stuhood merged commit e7cc575 into pantsbuild:master Jul 26, 2018

1 check passed

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

@Eric-Arellano Eric-Arellano deleted the Eric-Arellano:py3-fixes_netrc-retry-objects branch Jul 26, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

Python 3 fixes - fix netrc.py, retry.py, and test_objects.py (pantsbu…
…ild#6235)

* Fix netrc issue with builtins rename

* Fix TypeError exception message rewording between Py2 vs Py3

* Fix retry not reraising exception on Py3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment