-
Notifications
You must be signed in to change notification settings - Fork 384
Only mark new features as fetched after call goes through #2713
Conversation
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.
Hey @BasThomas, thanks for the PR.
I have couple of suggestions/questions.
(Probably outside this PR scope, but wouldn't make sense for fetch
to have an error handler)?
} | ||
let string = String(decoding: data, as: UTF8.self) | ||
DispatchQueue.main.async { [weak self] in | ||
guard let strongSelf = self else { return } |
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.
nit: I think there is no need to write strongSelf
. self
would work.
let message = """ | ||
"Expected a successfull or failing status code, but errored | ||
with \(error?.localizedDescription ?? "no error") for | ||
\(response?.description ?? "no response") |
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.
Did you mean httpResponse
(instead of response
)?
DispatchQueue.main.async { [weak self] in | ||
guard let strongSelf = self else { return } | ||
strongSelf.hasFetchedLatest = true | ||
strongSelf.latestMarkdown = string |
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.
why not?
strongSelf.latestMarkdown = String(decoding: data, as: UTF8.self)
let task = session.dataTask(with: url) { data, response, error in | ||
guard | ||
let httpResponse = response as? HTTPURLResponse, | ||
[200, 404].contains(httpResponse.statusCode), |
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.
I am not sure if we need to take into consideration 404
. If the status is 404
and rest of the guard passes, means that hasFetchedLatest
will be true
. Which, in IMO, shouldn't be. 🤔
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.
I think we need to care only about this range 200..<300
.
Sent with GitHawk |
It seems like we only try to check for new features once per version. That's cool, as to limit unnecessary network requests, but would silently not show any new features if that call would initially fail (eg. no network).
It now checks if it either succeeds (200) or can't find the features (404), and then communicate that it was fetched.