Closed Bug 1284843 Opened 4 years ago Closed 3 years ago

Reps: don't add quotes around strings

Categories

(DevTools :: Shared Components, enhancement, P1)

enhancement

Tracking

(firefox50 affected, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: linclark, Assigned: harry7)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 3 obsolete files)

The string rep adds quotes around strings. The console currently expects there to not be quotes around strings.
Blocks: 1283847
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
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?
Flags: needinfo?(odvarko)
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Actually, we wouldn't use the string rep directly, we might just output the string in that case
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
Resolved at our meeting. We might want to use quotes everywhere, but let's include Brian in the loop before making a decision.

Honza
Flags: needinfo?(odvarko)
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.
Flags: needinfo?(bgrinstead)
I don't have a strong opinion on quotes / no quotes but I'd value consistency with other tools in this case
Flags: needinfo?(bgrinstead)
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.
(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
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"}
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
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
Assignee: lclark → nobody
Status: ASSIGNED → NEW
Iteration: 50.4 - Aug 1 → ---
Priority: P1 → P2
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 ?
Also after we add this prop what should be the output of 
> console.log({bar: "foo"})
As per my understanding I made the changed and added a test. Please review and suggest If I am wrong somewhere.
Attachment #8775730 - Flags: review?(lclark)
Assignee: nobody → hems.india1997
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
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
Attachment #8775730 - Flags: review?(lclark) → review-
Made Necessary Changes and ran the mentioned tests.Works fine.
Attachment #8775730 - Attachment is obsolete: true
Attachment #8776630 - Flags: review?(lclark)
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
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
Attachment #8776630 - Flags: review?(lclark)
It's updated my repository and created the patch. It will be applied successfully now.
Attachment #8776630 - Attachment is obsolete: true
Attachment #8777460 - Flags: review?(lclark)
I've submitted this to our testing server, we'll see how it does.
It looks like it passed tests, so this is ready to go
Keywords: checkin-needed
Attachment #8777460 - Flags: review?(lclark) → review+
Attachment #8778381 - Attachment is obsolete: true
Attachment #8778381 - Flags: review?(lclark)
Is this resolved? If yes can you suggest me another good bug to work on ?
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/62dd82f4b4c0
Reps: Don't add quotes around strings. r=linclark
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/62dd82f4b4c0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.