Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Add support for multiple string URLs in IORouterRegistry #1214

Merged
merged 3 commits into from
Aug 8, 2018

Conversation

caisq
Copy link
Collaborator

@caisq caisq commented Aug 7, 2018

FEATURE


This change is Reviewable

@caisq caisq requested a review from pyu10055 August 7, 2018 03:25
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @pyu10055)


src/io/browser_http.ts, line 219 at r1 (raw file):

if (urlItem.startsWith(scheme))

if (!urlItem.startsWith(scheme)) ?

Copy link
Collaborator Author

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq)


src/io/browser_http.ts, line 219 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…
if (urlItem.startsWith(scheme))

if (!urlItem.startsWith(scheme)) ?

Oops. Sorry for that. And thanks for catching it. Unit test now added.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewed 5 of 5 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @caisq)


src/io/browser_http.ts, line 211 at r2 (raw file):

function isHTTPScheme(url: string) {
  for (const scheme of BrowserHTTPRequest.URL_SCHEMES) {
    if (url.startsWith(scheme)) {

consider use regex for the pattern matching.


src/io/browser_http.ts, line 226 at r2 (raw file):

    let isHTTP = true;
    if (Array.isArray(url)) {
      for (const urlItem of url) {

you can user isHTTP = url.every(urlItem => isHTTPSCheme(urlItem));

Copy link
Collaborator Author

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @pyu10055)


src/io/browser_http.ts, line 211 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

consider use regex for the pattern matching.

Done.


src/io/browser_http.ts, line 226 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

you can user isHTTP = url.every(urlItem => isHTTPSCheme(urlItem));

Done.

@caisq caisq merged commit f63d09e into tensorflow:master Aug 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants