Feature/issue #85 #91
Conversation
|
Looks great, thank you! I'll check this out this week. |
| using (var response = webRequest.GetResponse()) | ||
| { | ||
| using (var content = response.GetResponseStream()) | ||
| { | ||
| if (content == null) throw new ArgumentNullException($"Can't get content from URL: {uri}"); | ||
| using (var reader = new StreamReader(content)) | ||
| { | ||
| var version = reader.ReadToEnd().Trim(); | ||
| return version; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
You can merge these usings (omit the brackets of the first one) or if OK in this project also use the new using var notation. The same in GetLatestVersion().
There was a problem hiding this comment.
In my opinion current implementation is pretty simple and obvious
There was a problem hiding this comment.
I'll let you guys decide this issue. I grabbed already existing code to implement this part.
|
|
||
| public virtual string GetMatchingBrowserInstalledVersion() | ||
| { | ||
| throw new NotImplementedException(); |
There was a problem hiding this comment.
Hmm, I awaited this to be thrown when using FF with version: "matching_installed" but it didn't. What could be the issue?
|
@tteguayco @Piedone I've updated code a little bit, can you please review it? |
Looks great to me, @rosolko. Your naming convention is much better and clear. Could you check what @Piedone says about the exception ```NotImplementedException```` not being thrown when using a browser that does not support this new feature? |
|
Looks great! I've also tested it and works as it should. I now also get the exception for Firefox. |
3 participants
A
chromedriver.exethat matches the version of the installed Google Chrome browser on the user's machine can be now automatically downloaded.This Pull Request contains an addition to the README that shows how this new feature can be used.
Tested "on the fly", as I couldn't think of a unit test that makes sense (it depends on the machine which the project runs on).
Issue requesting feature: #85