Support string flag #7509
Support string flag #7509
Conversation
axinging
changed the title
axinging
force-pushed
the
string_flags
branch
3 times, most recently
from
8ab4a56
to
edbf645
Compare
| @@ -192,9 +196,9 @@ function parseValue(flagName: string, value: string): FlagValue { | |||
| return value === 'true'; | |||
| } else if (`${+ value}` === value) { | |||
| return +value; | |||
| } else { | |||
| return value; | |||
There was a problem hiding this comment.
For string, I'd prefer case sensitive.
There was a problem hiding this comment.
I agree with Yang. Case sensitive would be better for strings.
There was a problem hiding this comment.
On line 77, we set flags based on the url flags
if (this.urlFlags[flagName] != null) {
const flagValue = this.urlFlags[flagName];
These url flags are parsed by this.urlFlags[key] = parseValue(key, value);, However, parseValue makes the value lowercase with value = value.toLowerCase();. That makes all flags specified in the URL lowercase, and it's impossible to specify an uppercase url flag.
Should we use a different variable, valueLowercase, to store the lowercase value and then return the unmodified value here instead?
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 1 approvals obtained (waiting on @gyagp and @mattsoulanille)
tfjs-core/src/environment.ts line 200 at r1 (raw file):
Previously, gyagp (Yang Gu) wrote…
For string, I'd prefer case sensitive.
+1
| @@ -192,9 +196,9 @@ function parseValue(flagName: string, value: string): FlagValue { | |||
| return value === 'true'; | |||
| } else if (`${+ value}` === value) { | |||
| return +value; | |||
| } else { | |||
| return value; | |||
There was a problem hiding this comment.
I agree with Yang. Case sensitive would be better for strings.
axinging
force-pushed
the
string_flags
branch
2 times, most recently
from
1f8e9cd
to
8b1bf57
Compare
|
@gyagp @mattsoulanille |
| @@ -192,9 +196,9 @@ function parseValue(flagName: string, value: string): FlagValue { | |||
| return value === 'true'; | |||
| } else if (`${+ value}` === value) { | |||
| return +value; | |||
| } else { | |||
| return value; | |||
There was a problem hiding this comment.
On line 77, we set flags based on the url flags
if (this.urlFlags[flagName] != null) {
const flagValue = this.urlFlags[flagName];
These url flags are parsed by this.urlFlags[key] = parseValue(key, value);, However, parseValue makes the value lowercase with value = value.toLowerCase();. That makes all flags specified in the URL lowercase, and it's impossible to specify an uppercase url flag.
Should we use a different variable, valueLowercase, to store the lowercase value and then return the unmodified value here instead?
|
@gyagp @mattsoulanille , now URL string is also case seneitive, PTAL |
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is