Closed Bug 727967 Opened 13 years ago Closed 12 years ago

Convert Dict.jsm from/to JSON

Categories

(Toolkit :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: Yoric, Assigned: avp)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [mentor=Yoric][lang=js][good first bug])

Attachments

(1 file, 11 obsolete files)

3.99 KB, patch
Details | Diff | Splinter Review
Dict.jsm is quite useful to represent key/value pairs such as preferences. However, there is no good way of saving it to disk/restoring it from disk. Implementing Dict.prototype.toJSON and Dict.ofJSON would essentially solve the issue.
It's current toString is JSON-compatible, isn't it? There may be some edge cases it doesn't handle. It's also somewhat inefficient, but I guess there's no easy way around that given the key prefix.
Indeed, |toString| should be JSON-compatible, but |JSON.stringify| will not use it. Rather, |JSON.stringify| returns a string that cannot be properly parsed back to a |Dict| object.
So what is this bug about, then? I don't think it's feasible to change JSON.parse/stringify to properly round-trip Dict objects. Dict.jsm users must use json = JSON.stringify(JSON.parse(dict.toString())) and dict = new Dict(JSON.parse(json))
I hope you agree that it would be nicer and less error-prone to simply write json = JSON.stringify(dict) dict = Dict.ofJSON(json) To fix the behavior of |JSON.stringify|, we simply need to implement |json.prototype.toJSON|, with the exact same behavior as the current |json.prototype.toString|. |JSON.stringify| will use it. To implement |Dict.ofJSON|, we simply need a one-liner returning |new Dict(JSON.parse(json))|. Note: I did not mark this "good first bug" by accident. This can be done in 5 minutes + testsuite.
Ah, I didn't know that the toJSON hook already existed. Nevermind, then! (I think it would be nicer to change the constructor to assume a string argument is JSON, rather than adding a static ofJSON method.)
Hi David, Can I be assigned to this bug? Thanks
(In reply to Kevin Huynh from comment #6) > Hi David, > > Can I be assigned to this bug? Thanks With pleasure. Feel free to get in touch with me on IRC (irc.mozilla.org, channel #introduction ) if you have any question.
Assignee: nobody → kevinhuynh1
Any news about this? Or should I set the bug as unassigned?
Setting the bug as unassigned.
Assignee: kevinhuynh1 → nobody
Assignee: nobody → abhishekp.bugzilla
Hi Yorin, I have started working on the Dict.jsm file. I have edited the constructor and am pasting the constructor here. function Dict(aInitial) { if (aInitial === undefined) aInitial = {}; //my code begins here if(typeof aInitial =="string")//passed object is a string { //code to convert into JSON aInitial = JSON.parse(aInitial); } //my code ends here var items = {}, count = 0; // That we don't look up the prototype chain is guaranteed by Iterator. for (var [key, val] in Iterator(aInitial)) { items[convert(key)] = val; count++; } this._state = {count: count, items: items}; return Object.freeze(this); } is this part correct ? I tested it using xpcshell by pasting the constructor in the console. is that the right way to test the code or is there any other way through which I can test my code ? Also I am confused about the toString and toJSON part that you explained to me on IRC.Could you please explain it to me again. Thanks.
extremely sorry Yoric, I misspelled your name in the previous comment. really very sorry.
(In reply to Abhishek Potnis from comment #10) > Hi Yorin, > I have started working on the Dict.jsm file. I have edited the constructor > and am pasting the constructor here. > > function Dict(aInitial) { > if (aInitial === undefined) > aInitial = {}; > > //my code begins here > if(typeof aInitial =="string")//passed object is a string > { > //code to convert into JSON > aInitial = JSON.parse(aInitial); > } > //my code ends here > > var items = {}, count = 0; > // That we don't look up the prototype chain is guaranteed by Iterator. > for (var [key, val] in Iterator(aInitial)) { > items[convert(key)] = val; > count++; > } > this._state = {count: count, items: items}; > return Object.freeze(this); > } > > is this part correct ? That looks good. By the way, generally, for this kind of feedback, we prefer the following process: - extract the changes as a patch using mercurial; - attach the patch to the bug; - mark it as "feedback?" for the person who should provide feedback (in my case, you can use ":Yoric", for instance, or my full name). This will send an e-mail to the person who should give you feedback on the code. Later, when the patch is complete, use the same process but mark instead it as "review?". You can find more information here: https://developer.mozilla.org/en/Code_Review_FAQ > I tested it using xpcshell by pasting the constructor > in the console. > is that the right way to test the code or is there any other way through > which I can test my code ? It is a good start. You should also run test_dict.js to ensure that you have not broken it, as follows: cd your_object_directory make -C toolkit/content make -C browser make -C toolkit/content/tests/unit xpcshell-tests This will rebuild the JavaScript parts of your code and launch the xpcshell tests for toolkit/content. You can find more information about xpcshell tests here: https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests#Running_unit_tests > Also I am confused about the toString and toJSON part that you explained to > me on IRC. Could you please explain it to me again. It is actually quite simple. We want a new method |toJSON|. This method should return a string in JSON format representing the dictionary. As it turns out, the current version of method |toString| does exactly that. So we should basically: - rename the current |toString| into |toJSON|; - add a new method |toString| that just calls this new method |toJSON|. This will not change the behavior of |toString|, but this will let us free to change it at a later date if we want, without breaking |toJSON|. Does this answer your questions?
yes, thanks !
I have added the toJSON method and also made the toString call toJSON. I tested by pasting the entire code on xpcshell console. I could not execute the last command : make -C toolkit/content/tests/unit xpcshell-tests got an error :make: *** toolkit/content/tests/unit: No such file or directory. Stop.
Attachment #638103 - Flags: feedback?
Attachment #638103 - Flags: feedback? → feedback?(dteller)
(In reply to Abhishek Potnis from comment #14) > Created attachment 638103 [details] [diff] [review] > modified the constructor and added the toJSON method > > I have added the toJSON method and also made the toString call toJSON. > I tested by pasting the entire code on xpcshell console. > I could not execute the last command : make -C toolkit/content/tests/unit > xpcshell-tests > got an error :make: *** toolkit/content/tests/unit: No such file or > directory. Stop. Did you run this from the objdir?
yes, I did but there is no unit folder in the toolkit/content/tests
Comment on attachment 638103 [details] [diff] [review] modified the constructor and added the toJSON method Review of attachment 638103 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/content/Dict.jsm @@ +39,5 @@ > + { > + //code to convert into JSON > + aInitial = JSON.parse(aInitial); > + } > + Code looks good, but you will have to fix the layout. Something along the lines of if (typeof aInitial == "string") { aInitial = JSON.parse(aInitial); } Also, you will need to add your changes to the documentation (see the line |@param aInitial ... |). @@ +235,5 @@ > > /** > * Returns a string representation of this dictionary. > */ > + toJSON: function Dict_toJSON() { That's good. @@ +242,5 @@ > "}"; > }, > + toString: function Dict_toString() { > + Dict_toJSON(); > + }, Nope, this should be return this.toJSON();
Attachment #638103 - Flags: feedback?(dteller)
(In reply to Abhishek Potnis from comment #14) > Created attachment 638103 [details] [diff] [review] > modified the constructor and added the toJSON method > > I have added the toJSON method and also made the toString call toJSON. > I tested by pasting the entire code on xpcshell console. > I could not execute the last command : make -C toolkit/content/tests/unit > xpcshell-tests > got an error :make: *** toolkit/content/tests/unit: No such file or > directory. Stop. Remove the unit/ from the command line. xpcshell-tests is always invoked from tests/.
Attachment #638103 - Attachment is obsolete: true
Attachment #638114 - Flags: feedback?(dteller)
Comment on attachment 638114 [details] [diff] [review] Made the suggested changes in Dict.jsm Review of attachment 638114 [details] [diff] [review]: ----------------------------------------------------------------- I'm good with this patch if you make the final few changes. ::: toolkit/content/Dict.jsm @@ +34,1 @@ > */ Let me suggest the following: "if |aInitial| is a string, it is assumed to be JSON and parsed into an object". @@ +39,5 @@ > + > + if(typeof aInitial =="string") { > + aInitial = JSON.parse(aInitial); > + } > + We are pretty strict about layout, not (just) because we are psychorigid, but also because if everybody uses the same layout, this simplifies a lot reading for the next hundred persons who are going to try and understand the code. So, I am going to insist about the following points: - missing space after "if"; - missing space after "=="; - the closing "}" is not aligned. Could you please fix these? For more details, see https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide .
Attachment #638114 - Flags: feedback?(dteller)
Attachment #638114 - Attachment is obsolete: true
Attachment #638165 - Flags: review?(dteller)
Comment on attachment 638165 [details] [diff] [review] Made the suggested changes in Dict.jsm Review of attachment 638165 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Now, you need to write tests for the new features you have added. Once this is done, we will ask gavin to make the final review(s).
Attachment #638165 - Flags: review?(dteller)
Hi Yoric, Could you please guide me on where and how to start for writing the tests. Thanks.
(In reply to Abhishek Potnis from comment #23) > Could you please guide me on where and how to start for writing the tests. Sure thing. We use a xpcshell test to test the features of Dict.jsm. This test is in toolkit/content/tests/unit/test_dict.js . Now, you have added two features: 1/ constructing a dictionary from a JSON string; 2/ serializing a dictionary to a JSON string. So you should extend test_dict.js to test these two features. 1/ Testing that you can indeed construct a dictionary from a JSON string: - Construct a dictionary |d1| from an object (e.g. |{a:1, b:2, c:"foobar"}|); - Construct a second dictionary |d2| from a JSON string representing the same object: |JSON.stringify(({a:1, b:2, c:"foobar"})|; - Ensure that each key of |d1| appears in |d2| with the same value and that each key of |d2| appears in |d1| with the same value. See the manual of Dict.jsm https://developer.mozilla.org/en/JavaScript_code_modules/Dict.jsm for how to get the keys and the values of a dictionary. 2/ Testing that serialization works: - Construct a dictionary |d1| from an object (as above); - Serialize this dictionary to a JSON string by using |d1.toJSON()|; - Construct a dictionary |d2| from the JSON string; - As above, check that they have the exact same of keys and values.
Attachment #638301 - Flags: feedback?(dteller)
Comment on attachment 638301 [details] [diff] [review] extended test_dict.js to test for added features Review of attachment 638301 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Could you please ensure that the layout of your added tests matches the layout of the existing tests? If the tests pass, you should now request a review from :gavin.
Attachment #638301 - Flags: feedback?(dteller) → feedback+
Attachment #638301 - Attachment is obsolete: true
Attachment #638335 - Flags: feedback?(dteller)
Comment on attachment 638335 [details] [diff] [review] extended test_dict.js to test for added features Review of attachment 638335 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. You should now request reviews from :gavin.
Attachment #638335 - Flags: feedback?(dteller) → feedback+
Attachment #638165 - Flags: review?(gavin.sharp)
Attachment #638335 - Flags: review?(gavin.sharp)
review ping?
Comment on attachment 638165 [details] [diff] [review] Made the suggested changes in Dict.jsm >diff -r a12ce6b09f13 toolkit/content/Dict.jsm > function Dict(aInitial) { > if (aInitial === undefined) > aInitial = {}; >+ if(typeof aInitial =="string") { >+ aInitial = JSON.parse(aInitial); >+ } Indentation is wrong here, which makes the code look wrong. > /** > * Returns a string representation of this dictionary. > */ Fix the comment? s/string/JSON/ >+ toString: function Dict_toString() { >+ return this.toJSON(); Indentation is wrong here too (one extra space). r=me with those fixed.
Attachment #638165 - Flags: review?(gavin.sharp) → review+
Attachment #638335 - Flags: review?(gavin.sharp) → review+
I have fixed the indentation, but I did not get how to fix that comment. Should it be like "Returns a string/JSON representation of this dictionary" ? or should I add separate comments for the two functions |toJSON| and |toString| ?
Just replace "string" with "JSON" in the comment (that's what "s/string/JSON/" means). I don't think it's necessary to add any more comments, this is pretty straightforward.
Attached patch Converted Dict.jsm from/to JSON (obsolete) — Splinter Review
Attachment #638165 - Attachment is obsolete: true
Attachment #638335 - Attachment is obsolete: true
Attachment #657575 - Flags: review?(gavin.sharp)
Attachment #657575 - Flags: review?(gavin.sharp) → review+
Should I mark the patch as check-in required ?
Keywords: checkin-needed
Hi, What changes are required in the patch, so that the test passes ? How should I analyze this scenario ? Please help. Thanks
Abhishek, the failure indicated by Ryan leads to the error at: https://tbpl.mozilla.org/php/getParsedLog.php?id=14996672&tree=Try#error0 You should fix that error, launch the test locally to ensure that it now works, then mark the patch as checkin-needed.
Attached patch Converted Dict.jsm from/to JSON (obsolete) — Splinter Review
Attachment #657575 - Attachment is obsolete: true
Attachment #662474 - Flags: checkin?
Attachment #662474 - Flags: checkin?
Whiteboard: [mentor=Yoric][lang=js][good first bug] → [autoland-try][mentor=Yoric][lang=js][good first bug]
Whiteboard: [autoland-try][mentor=Yoric][lang=js][good first bug] → [autoland-in-queue][mentor=Yoric][lang=js][good first bug]
autoland neverland!
Keywords: checkin-needed
Whiteboard: [autoland-in-queue][mentor=Yoric][lang=js][good first bug] → [mentor=Yoric][lang=js][good first bug]
Pushed to Try. https://tbpl.mozilla.org/?tree=Try&rev=e6d648fea6f0 Also Abhishek, can you please edit your hgrc so that there's a space between your name and your email address? Thanks!
This is still failing tests. Please do not request checkin without a link to a green Try run. TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/content/tests/unit/test_dict.js | test failed (with xpcshell return code: 0), see following log: TEST-UNEXPECTED-FAIL | resource://gre/modules/Dict.jsm:38 | SyntaxError: JSON.parse: expected property name or '}' - See following stack: JS frame :: Dict@resource://gre/modules/Dict.jsm:38 JS frame :: test_serialize_dict_to_json_string@/Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/content/tests/unit/test_dict.js:279 JS frame :: run_test@/Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/content/tests/unit/test_dict.js:303 JS frame :: _execute_test@/Users/cltbld/talos-slave/test/build/xpcshell/head.js:315 JS frame :: @-e:1
Keywords: checkin-needed
extremely sorry for the inconvenience caused ! will look into it and fix the error. Sorry for marking it as checkin without testing it locally. Will mark as checkin needed only after testing locally ! very sorry !
Attached patch Rewrote toJSON method (obsolete) — Splinter Review
Attachment #662474 - Attachment is obsolete: true
Attachment #666554 - Flags: feedback?(dteller)
Comment on attachment 666554 [details] [diff] [review] Rewrote toJSON method Review of attachment 666554 [details] [diff] [review]: ----------------------------------------------------------------- (canceling review, you got my feedback on IRC) ::: toolkit/content/Dict.jsm @@ +28,5 @@ > * > * @param aInitial An object containing the initial keys and values of this > * dictionary. Only the "own" enumerable properties of the > * object are considered. > + * If |aInitial| is a string, it is assumed to be JSON and parsed into an object. Nitpick: please remove the white spaces at the end of the line.
Attachment #666554 - Flags: feedback?(dteller)
Attached patch Converted Dict.jsm from/to JSON (obsolete) — Splinter Review
Attachment #666554 - Attachment is obsolete: true
Attachment #666570 - Flags: feedback?(dteller)
Comment on attachment 666570 [details] [diff] [review] Converted Dict.jsm from/to JSON Review of attachment 666570 [details] [diff] [review]: ----------------------------------------------------------------- This should be good, except for a few typography issues. You can find more details about our coding guidelines at https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide You have my feedback+, provided tests pass, but could you please fix these issues? ::: toolkit/content/Dict.jsm @@ +240,5 @@ > + * Returns a JSON representation of this dictionary. > + */ > + toJSON: function Dict_toJSON() { > + var items = this._state.items; > + let JSONobj={}; Nitpick: whitespaces around "=". @@ +242,5 @@ > + toJSON: function Dict_toJSON() { > + var items = this._state.items; > + let JSONobj={}; > + for(let k in items) > + JSONobj[unconvert(k)] = items[k]; We generally: - add a whitespace after |for|; - add a "{" at the end of that line; - move the following block by two whitespaces. This simplifies reading the code. ::: toolkit/content/tests/unit/test_dict.js @@ +281,5 @@ > + do_check_eq(d1.get("a"), d2.get("a")); > + do_check_eq(d1.get("b"), d2.get("b")); > + do_check_eq(d1.get("c"), d2.get("c")); > +} > + Nitpick: I see 4 lines ending with whitespaces. Could you please remove them?
Attachment #666570 - Flags: feedback?(dteller) → feedback+
Attached patch Converted Dict.jsm from/to JSON (obsolete) — Splinter Review
Attachment #666570 - Attachment is obsolete: true
Attachment #666594 - Flags: review?(gavin.sharp)
Autoland Patchset: Patches: 662474 Branch: mozilla-central => try Insufficient permissions to push to try.
Comment on attachment 666594 [details] [diff] [review] Converted Dict.jsm from/to JSON >diff --git a/toolkit/content/Dict.jsm b/toolkit/content/Dict.jsm >+ toJSON: function Dict_toJSON() { >+ var items = this._state.items; >+ let JSONobj = {}; >+ for (let k in items) { >+ JSONobj[unconvert(k)] = items[k]; >+ } >+ return JSON.stringify(JSONobj); >+ }, This seems equivalent to JSON.stringify(this.items), can't you just use that?
Using JSON.stringify(this._state.items) is not letting me separate the ":" that is prefixed to the keys that is used to prevent collision with any built-ins. It was being possible to do so in my previous patch as I had used unconvert method. So can you please suggest an alternate way to do it ? Thanks.
Note that I suggested |this.items|, not |this._state.items|. The former is a getter that already does essentially what you were doing in your toJSON, AFAICT.
Ah, except that it returns an Generator, not an object, so that won't work.
Comment on attachment 666594 [details] [diff] [review] Converted Dict.jsm from/to JSON You should be able to use: let obj = {}; for (let [key, item] of Iterator(this._state.items)) { obj[unconvert(key)] = item; return JSON.stringify(obj); r=me with that.
Attachment #666594 - Flags: review?(gavin.sharp) → review+
Attached patch Converted Dict.jsm from/to JSON (obsolete) — Splinter Review
Attachment #666594 - Attachment is obsolete: true
Attachment #669969 - Flags: review?(gavin.sharp)
Comment on attachment 669969 [details] [diff] [review] Converted Dict.jsm from/to JSON The indentation is wrong in this patch (the line in the for loop needs to be indented). r=me with that fixed (you don't need to re-request review again, just attach an updated patch).
Attachment #669969 - Flags: review?(gavin.sharp) → review+
Attachment #669969 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(dteller)
Abhishek, you should really file a bug for Level 1 commit access so you can push your patches to Try. Myself or Yoric can vouch for you. http://www.mozilla.org/hacking/commit-access-policy/
Flags: needinfo?(dteller)
try was green.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: