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 support of get_full_details() for Throttled exceptions #4627

Merged

Conversation

ticosax
Copy link
Contributor

@ticosax ticosax commented Oct 26, 2016

Description

Since str objects are immutable, appending to existing str creates
in fact a new str instance.

Thus ErrorDetail.detail.code attribute is lost after str concatenation operation.

Copy link
Member

@tomchristie tomchristie left a comment

Good catch. One comment for consideration.

self.extra_detail_plural.format(wait=self.wait),
self.wait
)))))
self.detail.code = code
Copy link
Member

@tomchristie tomchristie Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we could instead push this above the super call, and then just pass detail and code to that without having to explicitly construct an ErrorDetail instance. (Eg use a style more likeUnsupportedMediaType above)

@ticosax ticosax force-pushed the fix-Trottling-error-code-extraction branch 2 times, most recently from bbb5dc9 to 15f85d6 Compare Oct 26, 2016
@ticosax
Copy link
Contributor Author

ticosax commented Oct 26, 2016

One comment for consideration.

Much better now, thx.

@ticosax
Copy link
Contributor Author

ticosax commented Oct 26, 2016

still not right. I will add another test.

@ticosax ticosax force-pushed the fix-Trottling-error-code-extraction branch from 15f85d6 to 2a996f5 Compare Oct 26, 2016
detail,
force_text(ungettext(self.extra_detail_singular.format(wait=self.wait),
self.extra_detail_plural.format(wait=self.wait),
self.wait))))
Copy link
Member

@tomchristie tomchristie Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I reckon that we should also tweak how we set self.wait as this scans slightly oddly at the moment. Let's not set self.wait inside this block. Instead just...

if detail is None:
    detail = force_text(self.default_detail)

if wait is not None:
    wait = math.ceil(wait)
    detail = ...

self.wait = wait
super(Throttled, self).__init__(detail, code)

Copy link
Contributor Author

@ticosax ticosax Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you got it

Copy link
Member

@tomchristie tomchristie Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this. Looking good!

Since `str` objects are immutable, appending to existing `str` creates
in fact a new `str` instance.

Thus `ErrorDetail.detail.code` attribute is lost after `str` concatenation operation.
@ticosax ticosax force-pushed the fix-Trottling-error-code-extraction branch from 2a996f5 to 3b34576 Compare Oct 26, 2016
@tomchristie tomchristie added this to the 3.5.2 Release milestone Nov 1, 2016
@tomchristie tomchristie added the Bug label Nov 1, 2016
@tomchristie tomchristie merged commit 97d8484 into encode:master Nov 1, 2016
@tomchristie
Copy link
Member

tomchristie commented Nov 1, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants