bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Reps: don't add quotes around strings

RESOLVED FIXED in Firefox 51

Status

DevTools
Shared Components
P1
enhancement
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: linclark, Assigned: harry7)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 51
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 affected, firefox51 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
The string rep adds quotes around strings. The console currently expects there to not be quotes around strings.
(Reporter)

Updated

2 years ago
Blocks: 1283847
Whiteboard: [devtools-html] [triage]

Updated

2 years ago
Blocks: 1263741

Updated

2 years ago
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
(Reporter)

Comment 1

2 years ago
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)
(Reporter)

Updated

2 years ago
Assignee: nobody → lclark
Status: NEW → ASSIGNED
(Reporter)

Comment 2

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

Updated

2 years ago
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)
(Reporter)

Comment 4

2 years ago
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)
(Reporter)

Comment 6

2 years ago
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
(Reporter)

Comment 8

2 years ago
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"}

Updated

2 years ago
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
(Reporter)

Comment 9

2 years ago
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
(Reporter)

Updated

2 years ago
Assignee: lclark → nobody
Status: ASSIGNED → NEW

Updated

2 years ago
Iteration: 50.4 - Aug 1 → ---
Priority: P1 → P2
(Assignee)

Comment 10

2 years ago
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 ?
(Assignee)

Comment 11

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

Comment 12

2 years ago
Created attachment 8775730 [details] [diff] [review]
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.
Attachment #8775730 - Flags: review?(lclark)

Updated

2 years ago
Assignee: nobody → hems.india1997
Status: NEW → ASSIGNED
Iteration: --- → 50.4 - Aug 1
Priority: P2 → P1
(Reporter)

Comment 13

2 years ago
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-
(Assignee)

Comment 14

2 years ago
Created attachment 8776630 [details] [diff] [review]
Bug 1284843 - Reps: don't add quotes around strings. r=linclark

Made Necessary Changes and ran the mentioned tests.Works fine.
Attachment #8775730 - Attachment is obsolete: true
Attachment #8776630 - Flags: review?(lclark)

Updated

2 years ago
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
(Reporter)

Comment 15

2 years ago
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)
(Assignee)

Comment 16

2 years ago
Created attachment 8777460 [details] [diff] [review]
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.
Attachment #8776630 - Attachment is obsolete: true
Attachment #8777460 - Flags: review?(lclark)
(Reporter)

Comment 17

2 years ago
Created attachment 8778381 [details]
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/
Attachment #8778381 - Flags: review?(lclark)
(Reporter)

Comment 18

2 years ago
I've submitted this to our testing server, we'll see how it does.
(Reporter)

Comment 19

2 years ago
It looks like it passed tests, so this is ready to go
Keywords: checkin-needed
(Reporter)

Updated

2 years ago
Attachment #8777460 - Flags: review?(lclark) → review+
(Reporter)

Updated

2 years ago
Attachment #8778381 - Attachment is obsolete: true
Attachment #8778381 - Flags: review?(lclark)
(Assignee)

Comment 20

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

Comment 21

2 years ago
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

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/62dd82f4b4c0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.