-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configurable internetUpUrl #1051
Conversation
dd312dc
to
5cd5ad6
Compare
5cd5ad6
to
6e35ac2
Compare
6e35ac2
to
273086a
Compare
273086a
to
6044f86
Compare
6044f86
to
eb714c0
Compare
eb714c0
to
51f0494
Compare
51f0494
to
b584135
Compare
b584135
to
376362b
Compare
376362b
to
a76d28e
Compare
a76d28e
to
55c2633
Compare
I've added @m-hulbert as a reviewer, given there is API commentary added. |
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 there could perhaps be more attention given to capitalisation within the API commentaries that have been added (e.g. url
-> URL
) but I will defer to DevEd/Docs team for input on that.
55c2633
to
0bc0838
Compare
@QuintinWillison I've updated the API doc for that option, fixing the mistake you mentioned and also adding a better description. |
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.
LGTM, thanks
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.
LGTM
Implementation is based on the one described by Lewis here. The only difference is that this implementation allows any success status code.
Includes some very basic URL parsing so that provided
internetAvailabilityUrl
can optionally contain a url scheme and/or query params.