Update Windows pre-release version handling. #9
Conversation
skewart
force-pushed
the
no-win-version-response
branch
7 times, most recently
from
da467fb to
5462224
Compare
lib/versions.js
Outdated
There was a problem hiding this comment.
These changes are required because Squirrel.Windows checks for equality locally. It never expects a 204 like Squirrel.Mac does, but instead that you return all the version info, and it will check to see if there is a newer version available. We already are checking to see if the tag is equal to the given tag in the osx update handler function, so this will be fine.
skewart
force-pushed
the
no-win-version-response
branch
from
5462224 to
b4cc907
Compare
lib/win-releases.js
Outdated
There was a problem hiding this comment.
In the existing code Nuts is excluding any non-digit characters from the version string when it reconstructs it. This means that you don't get the complete string to compare against the local one. Though, actually, if we leave the local build strings untouched then maybe it's not such a big deal. Hang on a second, lemme check something.
skewart
force-pushed
the
no-win-version-response
branch
from
9007245 to
1f20542
Compare
skewart
changed the title
skewart
force-pushed
the
no-win-version-response
branch
2 times, most recently
from
997960c to
9be7c1e
Compare
skewart
force-pushed
the
no-win-version-response
branch
2 times, most recently
from
e9911bf to
efe731c
Compare
lib/win-releases.js
Outdated
There was a problem hiding this comment.
Wrapping this in _.chain is kind of ugly. But I do like the idea of throwing an actually descriptive error rather than "Cannot read property 0 of null".
There was a problem hiding this comment.
FYI, lodash's thru is kind of like tap in ruby/rails https://lodash.com/docs#thru
There was a problem hiding this comment.
Should the error perhaps hint that the version info might be invalid? Something like Release missing valid version:
skewart
force-pushed
the
no-win-version-response
branch
2 times, most recently
from
51140b9 to
50b4ad0
Compare
lib/versions.js
Outdated
There was a problem hiding this comment.
I think you may have moved this out of the closure to allow for testing. Is there another way to to it? Would be nice not to have to move it. Maybe we could test the function that calls into extractChannel instead? idk.
There was a problem hiding this comment.
Yeah, testing is the only reason why I moved it out. It would be nicer to test the public function that calls this instead. The only reason I didn't do that is time. It would require mocking responses from Github. (And there's no benefit for this function being in the closure). Though that's probably a good thing to do in the end - and probably before submitting a PR into the main Nuts repo.
Hmm... I'll see if I can knock out a quick mock and test things a better way.
skewart
force-pushed
the
no-win-version-response
branch
from
50b4ad0 to
f815cb1
Compare
|
LGTM |
Update Windows pre-release version handling.
I'm really not sure why you'd want to return a 500 here as opposed to a 204, like you with with other platforms. Squirrel.Windows throws an error when it gets a 500 back when checking for releases.
Maybe I'll discover why Nuts returns a 500 instead of a 204 soon.