Skip to content
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

Clarify that noProxy expects a list of comma-separated entries. #1044

Closed
wants to merge 1 commit into from

Conversation

whimboo
Copy link
Contributor

@whimboo whimboo commented Aug 24, 2017

This change is Reviewable

@shs96c
Copy link
Contributor

shs96c commented Aug 24, 2017

@whimboo is a Mozilla employee, which is a w3c member organisation.

@whimboo
Copy link
Contributor Author

whimboo commented Aug 24, 2017

@shs96c I got my account validated and connected it to Github. Looks like we are set for a review now.

@shs96c shs96c modified the milestone: Level 1 Aug 25, 2017
@@ -1986,8 +1986,9 @@ <h2 data-dfn-for="NavigatorAutomationInformation">Interface</h2>
<tr>
<td><dfn><code>noProxy</code></dfn>
<td>array
<td>Lists the address for which the proxy should be bypassed when
the <a><code>proxyType</code></a> is "<code>manual</code>".
<td>Comma-separated list of hosts for which the proxy should be bypassed if
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Valid Values column tells us that this is not a comma-separated list, but a JSON List type:

A List containing any number of any of domains, IPv4 addresses, or IPv6 addresses.

If the driving force behind this is to make the implementation less fussy (for example, just a String that the implementation can do what it wants with), then you also need to modify the Valid Value column.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, so it means we are requiring something like {"noProxy": ["foo.bar", "127.0.0.1"]}?

With that the different driver implementations would have to join all entries into a single string with a separator the browser supports. I would be fine with that, and would update the PR to only make this entry sound similar to proxyAutoconfigUrl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shs96c any further feedback? If the last comment applies to how it should look like, we can close this issue. Or should we mention "JSON list" similar to other parts of the spec?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the spec is saying that we should have something like {"noProxy": ["foo.bar"]}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should we update to say "JSON list"? I can update my PR appropriately.

@shs96c
Copy link
Contributor

shs96c commented Aug 29, 2017

Closing this PR, as @whimboo requested. The spec is sufficiently clear.

@shs96c shs96c closed this Aug 29, 2017
@whimboo
Copy link
Contributor Author

whimboo commented Aug 29, 2017

@shs96c so no need to make it a "JSON List" what is the term we use at other places?

@andreastt
Copy link
Member

It already says that the valid input is a JSON List?

@shs96c
Copy link
Contributor

shs96c commented Aug 29, 2017

Yes. Under noProxy's "valid values"

A List containing any number of any of domains, IPv4 addresses, or IPv6 addresses.

With List linking to the JSON List.

@whimboo whimboo deleted the no_proxy branch August 30, 2017 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants