switch from the now deprecated to the new status page #2547
Conversation
r-52
force-pushed
the
r-new-status-page
branch
from
d664732 to
29f2bf9
Compare
the "old" status page https://status.github.com is no deprecated and GitHub switched to a statuspage.io implementation with https://githubstatus.com. The page mentions the deprecation. This PR switches the url to the new status page and changes the status API model for the new REST-response. The new API replaced the `good` state with the state `none`. And has more fields, that are probably not relevant for the App.
r-52
force-pushed
the
r-new-status-page
branch
from
29f2bf9 to
48701b9
Compare
| switch response.data.status.indicator { | ||
| case .none: | ||
| text = response.data.status.description | ||
| color = Styles.Colors.Green.medium.color |
There was a problem hiding this comment.
You could add a computed property on StatusType called color and this switch could be removed.
There was a problem hiding this comment.
I decided not to do this as this would need StatusType to import UIKit, which feels unnecessary, and a weird dependency in the networking layer.
| text = NSLocalizedString("Good", comment: "") | ||
| switch response.data.status.indicator { | ||
| case .none: | ||
| text = response.data.status.description |
There was a problem hiding this comment.
This should be able to be pulled out of the switch (DRY).
There was a problem hiding this comment.
Sorry for this mistake! Will fix it and thank you for the review!
| color = Styles.Colors.Red.medium.color | ||
| case .critical: | ||
| text = response.data.status.description | ||
| color = Styles.Colors.Red.medium.color |
There was a problem hiding this comment.
Should we introduce / can we use another color to disambiguate between .major and .critical?
| public let indicator: StatusType | ||
| public let description: String | ||
|
|
||
| public enum StatusType: String,Codable, CodingKey { |
There was a problem hiding this comment.
Super nit: no space after String,
| public let indicator: StatusType | ||
| public let description: String | ||
|
|
||
| public enum StatusType: String,Codable, CodingKey { |
There was a problem hiding this comment.
Can we rename the case none to case normal or similar? .none can silently be interpreted as Optional.none when working with an optional enum of this type, which can be hard to debug. Would require manually handling the decoding though.
There was a problem hiding this comment.
Decided against changing this for now as it would also "change" the API, which might be confusing, and certainly would be more confusing in most cases.
|
Nice work @romankl! |
Huddie
changed the title
Sherlouk
added
😴 awaiting changes
|
Hi @romankl, do you want to take another look at the comments? Can I help out? |
|
From @zhongwuzw:
|
|
@romankl Any interest in finishing this little bit of work of as per the above remarks? @rnystrom @BasThomas Just as a reminder, the deadline is moving in quickly (current page shuts down on Monday) |
|
@BasThomas @Sherlouk What’s left on this? Just www? Sent with GitHawk |
|
That and some more comments. Will work on that this weekend and merge. Sent with GitHawk |
|
Okay haha, I was gonna offer the same. Just to get it out. Enjoy! Sent with GitHawk |
|
Changes addressed, thanks so much for the initial push @romankl! |
7 participants
the "old" status page https://status.github.com is no deprecated and GitHub switched to a statuspage.io implementation with https://githubstatus.com. The page mentions the deprecation.
This PR switches the url to the new status page and changes the status API model for the new REST- response.
The new API replaced the
goodstate with the statenone. And has more fields, that are probablynot relevant for the App.
Previous response:
{"status":"good","last_updated":"2018-12-06T17:09:57Z"}New response:
{"page":{"id":"kctbh9vrtdwd","name":"GitHub","url":"https://www.githubstatus.com","time_zone":"Etc/UTC","updated_at":"2018-12-12T05:38:30.535Z"},"status":{"indicator":"none","description":"All Systems Operational"}}New page in SafariVC:
fixes: #2546