Only mark new features as fetched after call goes through #2713
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.