-
Notifications
You must be signed in to change notification settings - Fork 24
Bugfixes #14
Bugfixes #14
Conversation
|
@tfheen Would it be possible to rebase off the latest master? Would be happy to merge then. |
|
Sure, that should be possible. We have a bunch of other fixes in the pipe too, I'll get you PRs for those too. |
|
Thanks! Appreciate it.
…On Fri, Apr 7, 2017 at 12:52 AM Tollef Fog Heen ***@***.***> wrote:
Sure, that should be possible. We have a bunch of other fixes in the pipe
too, I'll get you PRs for those too.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AY_nqKvE4rbo5dbctPGkYZZXf32Gn2vbks5rtetFgaJpZM4IseCU>
.
|
self is an implied parameter. Avoids backtrace.
Call __getattr__ directly, else we end up calling ourselves which ends in tears.
…r,header} as their create_ counterparts
…e called None from the API
Names can (often) contain slashes, so we need to make sure those are encoded as well.
|
@copyconstruct: sorry it took a while to get around to this. Let me know if you would rather have this split out into a bunch of PRs, I can do that if you prefer. |
Fastly's API doesn't properly differentiate between healthcheck-as-a-string and as healthcheck-as-an-object, which means you will get healthcheck == '' back when it says "(none)" in the UI.
fastly/__init__.py
Outdated
| @property | ||
| def healthcheck(self): | ||
| return self._conn.get_healthcheck(self.service_id, self.version, self.healthcheck) | ||
| if self.__getattr__('healthcheck') is None or self.__getattr__('healthcheck') == '': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simplified to if not self.__getattr__('healthcheck'):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, fixed. A bit damaged by writing ruby. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be possible you didn't push up the change? Because it still shows the old diff to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right, it seems like I only added it to my master branch, not the bugfixes branch.
(Fixed now.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks! have merged.
|
@copyconstruct, any chance for a new review here (or possibly a merge)? Happy to make changes if you want those, but it'd be great to see movement on the project too. |
|
Thanks for merging, much appreciated! |
A small selection of bugfixes. I don't believe anything in here is controversial in any way.