Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Allow unknown numerical types for indent parameter #29

Merged
merged 2 commits into from

2 participants

@kini
Owner

This was causing failures in software which passes objects which are
neither of type str, int, or long, but rather a custom numerical class,
as the value of the indent parameter. Since this custom numerical class
understands multiplication by strings, it should be accepted by
simplejson's encoder and treated as a numerical argument.

... is what it says in the commit message. Of course, I make no such bold claim about what "should" happen, but am just submitting this pull request to see if you agree :) This is in some sense a regression because everything worked fine before string arguments for the indent parameter became supported, and the docstring suggests that backwards compatibility was aimed for.

@kini
Owner

WTF? Sorry, that code doesn't even run. Lemme fix that...

@kini kini Allow unknown numerical types for indent parameter
This was causing failures in software which passes objects which are
neither of type str, int, or long, but rather a custom numerical class,
as the value of the indent parameter. Since this custom numerical class
understands multiplication by strings, it should be accepted by
simplejson's encoder and treated as a numerical argument.
627af99
@kini
Owner

OK, should be fixed now. Sorry about the noise...

@etrepum
Owner

How about you also write a test for this so that there are no future regressions, and I'll go ahead and take that.

@kini
Owner

Great! Do you want a test that tests the situation I described, or just one which shows the regression? For example, instead of an actual numeric type, I could test that an object of the following type is accepted, which is a more minimal test case and easier to write, but I guess somehow less meaningful:

class Foo(object):
    def __mul__(self, other):
        return ''
@etrepum
Owner

I'd prefer to have something as close as possible to your use case, since the intent was always really just to support integers. Note that float and Decimal do not implement string multiplication, so we're not even going for generic number support.

@kini
Owner

Hmm. Well, my actual use case is quite convoluted and involves a huge amount of code that is not even entirely written in Python - you can see the relevant segment here if you're interested - so I won't try to simplify my actual use case. I'll make something along the same general lines, though.

@etrepum
Owner

I guess the real question is - why are you using this to specify the number of spaces to indent a JSON document?

@kini
Owner

We have a Python-based user console in which integer literals are hooked in the parser and turned into our custom integer-like objects so that they can interact better with the array of mathematical objects we provide. This means that if, for example, you type json.dumps(foo, indent=4) into the console, it is internally parsed as json.dumps(foo, indent=Integer(4)). So generally we try to make our custom objects do anything that standard Python ints can do. It's not that we particularly want to use our objects with simplejson, it's just that users may happen to do so when using our software.

I only happened to catch this because exactly such a line happened to appear in our test suite, and fails with the latest simplejson whereas it does not fail with the json module shipped with Python 2.7.2, making this technically a regression, IMO. Of course, you may consider this scenario not worth changing simplejson's code for, in which case we will need to modify our test suite, but it could still bite users who type in similar code in our console, and I think it's more elegant to patch simplejson in this way than to write code special-casing simplejson calls in our parser hooks...

@etrepum
Owner

That sounds like a reasonable use case. It's worth a small change in the code, I just want to make sure that once it's fixed it will stay fixed for you.

@kini
Owner

Great, I'm glad you think so. Here's a commit with a test and some documentation.

@kini

In making the class for the test, I noticed that I needed this further change. In my original use case, the class, or perhaps some other mechanism in the environment there, seems to be smart enough even to be able to handle ' ' * indent, but my toy example (and thus perhaps other users' examples) cannot. This diff chunk is not a regression fix, though, because such objects wouldn't have worked anyway before the indent parameter to the JSONEncoder constructor was made a string instead of an int, as far as I understand. (?)

Owner

I take that back. Such objects would have worked in the old version of simplejson, because in the old version, the indent parameter is never directly multiplied by ' '. The relevant line is

newline_indent = '\n' + (' ' * (_indent * _current_indent_level))

The fake integer turns into a real integer (by multiplication with a real integer in the "correct" order) before it is multiplied by ' '. So this is indeed part of the regression fix.

@etrepum etrepum merged commit 51744f7 into simplejson:master
@kini
Owner

Thanks!

@jperkin jperkin referenced this pull request from a commit in joyent/pkgsrc
wiz Update to 2.5.2:
Version 2.5.2 released 2012-05-10

* Fix for regression introduced in 2.5.1
  simplejson/simplejson#35

Version 2.5.1 released 2012-05-10

* Support for use_decimal=True in environments that use Python
  sub-interpreters such as uWSGI
  simplejson/simplejson#34

Version 2.5.0 released 2012-03-29

* New item_sort_key option for encoder to allow fine grained control of sorted
  output

Version 2.4.0 released 2012-03-06

* New bigint_as_string option for encoder to trade JavaScript number precision
  issues for type issues.
  simplejson/simplejson#31

Version 2.3.3 released 2012-02-27

* Allow unknown numerical types for indent parameter
  simplejson/simplejson#29

Version 2.3.2 released 2011-12-30

* Fix crashing regression in speedups introduced in 2.3.1

Version 2.3.1 released 2011-12-29

* namedtuple_as_object now checks _asdict to ensure that it
  is callable.
  simplejson/simplejson#26

Version 2.3.0 released 2011-12-05

* Any objects with _asdict() methods are now considered for
  namedtuple_as_object.
  simplejson/simplejson#22
6bc94ea
@jsonn jsonn referenced this pull request from a commit in jsonn/pkgsrc
wiz Update to 2.5.2:
Version 2.5.2 released 2012-05-10

* Fix for regression introduced in 2.5.1
  simplejson/simplejson#35

Version 2.5.1 released 2012-05-10

* Support for use_decimal=True in environments that use Python
  sub-interpreters such as uWSGI
  simplejson/simplejson#34

Version 2.5.0 released 2012-03-29

* New item_sort_key option for encoder to allow fine grained control of sorted
  output

Version 2.4.0 released 2012-03-06

* New bigint_as_string option for encoder to trade JavaScript number precision
  issues for type issues.
  simplejson/simplejson#31

Version 2.3.3 released 2012-02-27

* Allow unknown numerical types for indent parameter
  simplejson/simplejson#29

Version 2.3.2 released 2011-12-30

* Fix crashing regression in speedups introduced in 2.3.1

Version 2.3.1 released 2011-12-29

* namedtuple_as_object now checks _asdict to ensure that it
  is callable.
  simplejson/simplejson#26

Version 2.3.0 released 2011-12-05

* Any objects with _asdict() methods are now considered for
  namedtuple_as_object.
  simplejson/simplejson#22
b0726e7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 24, 2012
  1. @kini

    Allow unknown numerical types for indent parameter

    kini authored
    This was causing failures in software which passes objects which are
    neither of type str, int, or long, but rather a custom numerical class,
    as the value of the indent parameter. Since this custom numerical class
    understands multiplication by strings, it should be accepted by
    simplejson's encoder and treated as a numerical argument.
Commits on Feb 27, 2012
  1. @kini
This page is out of date. Refresh to see the latest.
Showing with 45 additions and 3 deletions.
  1. +4 −2 simplejson/encoder.py
  2. +41 −1 simplejson/tests/test_dump.py
View
6 simplejson/encoder.py
@@ -170,8 +170,10 @@ def __init__(self, skipkeys=False, ensure_ascii=True,
self.use_decimal = use_decimal
self.namedtuple_as_object = namedtuple_as_object
self.tuple_as_array = tuple_as_array
- if isinstance(indent, (int, long)):
- indent = ' ' * indent
+ try:
+ indent = indent * ' '
+ except TypeError:
+ pass
self.indent = indent
if separators is not None:
self.item_separator, self.key_separator = separators
View
42 simplejson/tests/test_dump.py
@@ -24,4 +24,44 @@ def test_ordered_dict(self):
# http://bugs.python.org/issue6105
items = [('one', 1), ('two', 2), ('three', 3), ('four', 4), ('five', 5)]
s = json.dumps(json.OrderedDict(items))
- self.assertEqual(s, '{"one": 1, "two": 2, "three": 3, "four": 4, "five": 5}')
+ self.assertEqual(s, '{"one": 1, "two": 2, "three": 3, "four": 4, "five": 5}')
+
+ def test_indent_unknown_type_acceptance(self):
+ """
+ A test against the regression mentioned at `github issue 29`_.
+
+ The indent parameter should accept any type which pretends to be
+ an instance of int or long when it comes to being multiplied by
+ strings, even if it is not actually an int or long, for
+ backwards compatibility.
+
+ .. _github issue 29:
+ http://github.com/simplejson/simplejson/issue/29
+ """
+
+ class AwesomeInt(object):
+ """An awesome reimplementation of integers"""
+
+ def __init__(self, *args, **kwargs):
+ if len(args) > 0:
+ # [construct from literals, objects, etc.]
+ # ...
+
+ # Finally, if args[0] is an integer, store it
+ if isinstance(args[0], int):
+ self._int = args[0]
+
+ # [various methods]
+
+ def __mul__(self, other):
+ # [various ways to multiply AwesomeInt objects]
+ # ... finally, if the right-hand operand is not awesome enough,
+ # try to do a normal integer multiplication
+ if hasattr(self, '_int'):
+ return self._int * other
+ else:
+ raise NotImplementedError("To do non-awesome things with"
+ " this object, please construct it from an integer!")
+
+ s = json.dumps(range(3), indent=AwesomeInt(3))
+ self.assertEqual(s, '[\n 0,\n 1,\n 2\n]')
Something went wrong with that request. Please try again.