feat(jsonFilter): add optional arg to define custom indentation #9771
Conversation
also change toJson function to accomodate passing in of number of spaces
attribute id missing d
| @@ -969,7 +969,8 @@ function toJsonReplacer(key, value) { | |||
| */ | |||
| function toJson(obj, pretty) { | |||
| if (typeof obj === 'undefined') return undefined; | |||
| return JSON.stringify(obj, toJsonReplacer, pretty ? ' ' : null); | |||
| var spaces = pretty ? (angular.isNumber(pretty) ? new Array(pretty + 1).join(' ') : ' ') : null; | |||
There was a problem hiding this comment.
A little far-fetched, but you can't use pretty-printing with 0-space indentation.
Just saying...
There was a problem hiding this comment.
Yeah you're right, I didn't think of that edge case, will push a slight refactor.
it is better to use an explicit number (instead of true), so that the default behaviour of jsonFilter does not change when / if the default indentation of toJson changes.
due to missing matching open parens
missing matching close parens
|
there are 9 commits for this, wow o_o |
| return function(object) { | ||
| return toJson(object, true); | ||
| return function(object, spacing) { | ||
| if (isUndefined(spacing)) spacing = 2; |
There was a problem hiding this comment.
if (isUndefined(spacing)) {
spacing = 2;
}
|
It basically looks good to me if you clean up the nits |
|
Should I move the parseInt logic from the toJson function into the filter or leave it as is? |
|
@doshprompt, parseint isn't needed at all. what would we use that for? |
| @@ -969,7 +969,7 @@ function toJsonReplacer(key, value) { | |||
| */ | |||
| function toJson(obj, pretty) { | |||
| if (typeof obj === 'undefined') return undefined; | |||
| return JSON.stringify(obj, toJsonReplacer, pretty ? ' ' : null); | |||
| return JSON.stringify(obj, toJsonReplacer, pretty ? parseInt(pretty) || 2 : null); | |||
There was a problem hiding this comment.
honestly this should just be JSON.stringify(obj, toJsonReplacer, pretty); --- browsers don't care if pretty is null or undefined or the empty string
There was a problem hiding this comment.
I think the idea is to keep backward compatibility by defaulting to 2 spaces when pretty is truthy, while allowing arbitrary indentations as well.
(Although, the way it is implemented, won't allow 0 indentation.)
There was a problem hiding this comment.
the old stuff was wrong too --- if anything it should be pretty === true ? 2 : pretty
There was a problem hiding this comment.
we don't need parseInt in here anywhere.
There was a problem hiding this comment.
I agree that the previous implementation was wrong, but I don't see how we can retain backward compatibility without parseInt (or a check for whether pretty is a number).
Unless we don't care retaining backward compatibility.
There was a problem hiding this comment.
parseInt isn't needed for backwards compatibility, it wasn't used before
There was a problem hiding this comment.
realistically, nobody is passing objects or NaN or other weird stuff to this, and if they are, who cares? it's not like it's a critical part of anyone's app
There was a problem hiding this comment.
Not sure I was clear before, so here is what I mean:
Old implementation:
pretty -> falsy: Create "ugly" JSON.
pretty -> truthy: Create pretty JSON with 2 spaces indentation.
New implementation:
pretty -> falsy: Create "ugly" JSON.
pretty -> truthy:
pretty -> number (say X): Create pretty JSON with X spaces indentation.
pretty -> non-numeric: Create pretty JSON with 2 spaces indentation (default).
The parseInt() is used for detecting true as well as other truthy, non-numeric values (although the latter would be rare).
I agree that it would be probably better to allow the user to pass anything as pretty (e.g. custom indentation string) and default to 2 spaces only if pretty === true, but it would be a breaking change (in the sense that it would work differently than the previous version for specific inputs).
That said (and since it is indeed not expected to be a critical aspect of anyone's app), if you think we should go with pretty === true ? 2 : pretty it sounds good to me.
@doshprompt:
Could you update the PR with the above change ?
It would be also nice to update the docs (for toJson() and json filter).
There was a problem hiding this comment.
parseInt() isn't needed for detecting numbers. But regardless, we don't need to worry about the "truthy vs true" thing, because it was a dumb idea to begin with and it's not going to matter intensely to anyone who uses it
also update docs
also change toJson function to accomodate passing in of number of spaces Closes angular#9771
also change toJson function to accomodate passing in of number of spaces