1284843 - Reps: don't add quotes around strings

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Lin Clark [:linclark]]

The string rep adds quotes around strings. The console currently expects there to not be quotes around strings.

Comment 1

[Lin Clark [:linclark]]

So I'm realizing that sometimes we need the quotes around strings, like when a string is a prop in a grip array.

I wonder if it makes more sense to special case the console output. If the top level grip is a string, then just use the String rep directly, otherwise pass it through Rep. What do you think?

Comment 2

[Lin Clark [:linclark]]

Actually, we wouldn't use the string rep directly, we might just output the string in that case

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Resolved at our meeting. We might want to use quotes everywhere, but let's include Brian in the loop before making a decision.

Honza

Comment 4

[Lin Clark [:linclark]]

Currently the output for console.log("foo", "bar") is

> foo bar

Brian, how do you fell about it being:

> "foo" "bar"

On the one hand, handling it like that would mean we don't need to special case those kinds of strings. On the other hand, this would break from what other tools like Chrome and the Node REPL do.

Comment 5

[Brian Grinstead [:bgrins]]

I don't have a strong opinion on quotes / no quotes but I'd value consistency with other tools in this case

Comment 6

[Lin Clark [:linclark]]

Brian and I discussed this in person and I think we have a good solution. There could be a prop on the String rep that is passed in, like useQuotes. 

In the ConsoleAPICall component, we would check before passing the grip to Rep to see if it's a string. If it is, we wouldn't use Rep for that value. Instead, we'd use StringRep directly. This way Rep doesn't have to know about the useQuotes prop.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Brian Grinstead [:bgrins] from comment #5)
> I don't have a strong opinion on quotes / no quotes but I'd value
> consistency with other tools in this case
Yes, I like the consistency too (that's the main goal of Reps).

(In reply to Lin Clark [:linclark] from comment #6)
> Brian and I discussed this in person and I think we have a good solution.
> There could be a prop on the String rep that is passed in, like useQuotes. 
> 
> In the ConsoleAPICall component, we would check before passing the grip to
> Rep to see if it's a string. If it is, we wouldn't use Rep for that value.
> Instead, we'd use StringRep directly. This way Rep doesn't have to know
> about the useQuotes prop.
Just curious, what is the reason to sometimes use quotes (useQuotes=true)
and sometimes not (useQuotes=false)? Why we are not consistent everywhere
across the entire UI?

Honza

Comment 8

[Lin Clark [:linclark]]

It is to match current behavior of the tool and the behavior of all tools that I've checked.

No quotes:
> console.log("foo")
> foo

With quotes:

> "foo" //Evaluation result
> "foo"

or 

> console.log({bar: "foo"})
> Object {bar: "foo"}

Comment 9

[Lin Clark [:linclark]]

What needs to be done for this issue:

- add a "useQuotes" prop to StringRep
- add a test for the "useQuotes" prop in the StringRep test

Comment 10

[Hemanth Kumar Veeranki [:harry7]]

I Would like to work on this Issue. Can someone give me some more info as to what exactly does adding a prop mean and what type of prop should useQuotes be ?

Comment 11

[Hemanth Kumar Veeranki [:harry7]]

Also after we add this prop what should be the output of 
> console.log({bar: "foo"})

Comment 12

[Hemanth Kumar Veeranki [:harry7]]

Attachment: Bug 1284843 - Reps: don't add quotes around strings. r=linclark

As per my understanding I made the changed and added a test. Please review and suggest If I am wrong somewhere.

Comment 13

[Lin Clark [:linclark]]

Comment on attachment 8775730 [details] [diff] [review]
Bug 1284843 - Reps: don't add quotes around strings. r=linclark

Review of attachment 8775730 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch. Review comments below.

In addition to the comments below, please make sure to run the webconsole, json viewer, and dom panel tests once you've made the changes. We should make sure that all of those pass with this change.

::: devtools/client/shared/components/reps/string.js
@@ +33,4 @@
>        let croppedString = this.props.cropLimit ?
>          cropMultipleLines(text, this.props.cropLimit) : cropMultipleLines(text);
>  
> +      let finalString  = (this.props.useQuotes == false )?

There don't need to be parenthesis around the condition and you don't need to check `== false`. You could just do this.props.useQuotes ? "\"" + croppedString + "\"" : croppedString.

However, as I'm thinking about it more, since most strings will use quotes, it's probably better for this property to be omitQuotes rather than useQuotes. Then it would be `this.props.omitQuotes ? croppedString : "\"" + croppedString + "\""`

Also, let's go with formattedString, not finalString

::: devtools/client/shared/components/test/mochitest/test_reps_string.html
@@ +27,4 @@
>      yield testMultiline();
>      yield testMultilineOpen();
>      yield testMultilineLimit();
> +    yield testuseQuotes();

The U in "Use" should be capitalized

@@ +49,4 @@
>      is(renderedComponent.textContent, "\"aaaaaaaaaaaaaaaaaaaaa\nbbbbbbbbbbbbbbbbbbb\ncccccccccccccccc\n\"", "String rep has expected text content for multiline string when open");
>    }
>  
> +  function testuseQuotes(){

U in use should be capitalized

@@ +49,5 @@
>      is(renderedComponent.textContent, "\"aaaaaaaaaaaaaaaaaaaaa\nbbbbbbbbbbbbbbbbbbb\ncccccccccccccccc\n\"", "String rep has expected text content for multiline string when open");
>    }
>  
> +  function testuseQuotes(){
> +     const renderedComponent = renderComponent(StringRep.rep, { object: getGripStub("testuseQuotes"), useQuotes:true });

Please run this against ESLint. I'm fairly certain it will throw an error because there should be a space between useQuotes and true

Comment 14

[Hemanth Kumar Veeranki [:harry7]]

Attachment: Bug 1284843 - Reps: don't add quotes around strings. r=linclark

Made Necessary Changes and ran the mentioned tests.Works fine.

Comment 15

[Lin Clark [:linclark]]

Comment on attachment 8776630 [details] [diff] [review]
Bug 1284843 - Reps: don't add quotes around strings. r=linclark

Review of attachment 8776630 [details] [diff] [review]:
-----------------------------------------------------------------

Overall it looks good. Unfortunately the patch does not apply. If you post a new one that does apply, I can push it up to the test server.

> $ git apply ../gecko-dev/diff.patch
> ../gecko-dev/diff.patch:41: space before tab in indent.
>      	 return "aaaaaaaaaaaaaaaaaaaaa\nbbbbbbbbbbbbbbbbbbb\ncccccccccccccccc\n";
error: patch failed: devtools/client/shared/components/reps/string.js:33
error: devtools/client/shared/components/reps/string.js: patch does not apply

Comment 16

[Hemanth Kumar Veeranki [:harry7]]

Attachment: Bug 1284843 - Reps: don't add quotes around strings. r=linclark

It's updated my repository and created the patch. It will be applied successfully now.

Comment 17

[Lin Clark [:linclark]]

Attachment: Bug 1284843 - Reps: don't add quotes around strings.

Review commit: https://reviewboard.mozilla.org/r/69700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69700/

Comment 18

[Lin Clark [:linclark]]

I've submitted this to our testing server, we'll see how it does.

Comment 19

[Lin Clark [:linclark]]

It looks like it passed tests, so this is ready to go

Comment 20

[Hemanth Kumar Veeranki [:harry7]]

Is this resolved? If yes can you suggest me another good bug to work on ?

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/62dd82f4b4c0
Reps: Don't add quotes around strings. r=linclark

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62dd82f4b4c0