dimagi/jsonobject

DictProperty doesn't support pop method

dannyroberts opened this issue · 4 comments

Solution

  • The change itself should be a 3- to 5-liner in SimpleDict, adding a pop method. As the documentation says, SimpleDict "Re-implements destructive methods of dict to use only __setitem__ and __getitem__ and __delitem__", so your pop method should use just only those three operations.

  • Then add a short test to the tests file, maybe near here.

  • Once done you can push to a branch and let travis run tests for you; or you can run them locally with following the example of the travis file. (I'd make a virtualenv first.)

  • If you want to run more tests against it, then checkout https://github.com/dimagi/couchdbkit with

    $ git clone https://github.com/dimagi/couchdbkit
    $ git checkout jsonobject  # our special branch that uses jsonobject instead of original schemas

    and then run the tests as shown in the travis file. (Again, I'd make another separate virtualenv.) If any of those fails (I doubt they will) feel free to ping; we want to add a test to jsonobject itself for any test in jsonobject-couchdbkit that fails.

Broader note

  • (Totally optional, may help someone else not experience the confusion and digging you did) In order to make this not fail on other methods in the dict interface, you may want to scan that list for any other destructive operations (destructive meaning changes the dict in anyway–append is destructive, keys is not) I may have missed and either implement them as well or at add a function that will raise an exception or log a warning or some such thing.

(also feel free to add documentation to the code in the form of docstrings for anything you felt you had to figure out yourself, that doesn't fully match what I told you to expect, or is just poorly worded)

Thanks for all the info Danny, PR here: #117.
I could use some feedback on the tests. (I also haven't run the couchdbkit tests.)

The associated PR seems to have since been merged. I guess this issue can then be closed?

Ah yes. Thanks @adrianbn!