Modified RobustNetworkOperation to send a UserAgent string#1492
Modified RobustNetworkOperation to send a UserAgent string#1492
Conversation
…prevent CI test failure.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and tombogle).
SIL.Core.Desktop/Network/RobustNetworkOperation.cs line 131 at r1 (raw file):
{ client.Proxy = proxy; client.Headers[HttpRequestHeader.UserAgent] = DefaultUserAgent;
Probably wants some comments.
Why do we need this?
Why do we only add it on this request?
Why do we want this particular useragent string as the default?
tombogle
left a comment
There was a problem hiding this comment.
@tombogle made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on andrew-polk and ddaspit).
SIL.Core.Desktop/Network/RobustNetworkOperation.cs line 131 at r1 (raw file):
Previously, andrew-polk wrote…
Probably wants some comments.
Why do we need this?
Why do we only add it on this request?
Why do we want this particular useragent string as the default?
Made the CI test pass. I didn't look at anything else. This was the one that was needed to fix the breaking test. ChatGPT said that was a good useragent string, and it worked. I just barely tried to digest its meaning.
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and tombogle).
SIL.Core.Desktop/Network/RobustNetworkOperation.cs line 131 at r1 (raw file):
Previously, tombogle (Tom Bogle) wrote…
Made the CI test pass. I didn't look at anything else. This was the one that was needed to fix the breaking test. ChatGPT said that was a good useragent string, and it worked. I just barely tried to digest its meaning.
Does it make sense to have a way for the tests to pass in a useragent string?
Maybe I'm just being dense, but I don't understand why it is ok to have production code suddenly sending a default useragent string which might not be correct.
|
Previously, andrew-polk wrote…
If the tests need it, it might also be the case that there could be situations where it is also required in production. It does feel like a bit of a hack (now that I'm taking a little time to almost understand it). We could make it an optional parameter to the method and make the constant public in case a real client needed to make it work and just wanted an easy way to spoof it. |
|
Previously, tombogle (Tom Bogle) wrote…
I think maybe I've come up with a satisfactory approach and really great comments. |
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 3 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ddaspit and tombogle).
SIL.Core.Desktop/Network/RobustNetworkOperation.cs line 131 at r1 (raw file):
and really great comments
exceptionally great
…fo to allow a client to mimic a real browser if necessary. Also improved the comments
771faee to
fe2a8e8
Compare
andrew-polk
left a comment
There was a problem hiding this comment.
@andrew-polk reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on ddaspit).
Trying to see if this might prevent CI test failure.
This change is