-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stringify cson with no indent #37
Conversation
serializedValue = visitNode value, bracesRequired: !indent | ||
if indent | ||
serializedValue = if isObject value | ||
"\n#{ indentLines serializedValue }" | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation here doesn't seem to align
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats the proper place for the else. Its matching if is only two lines above, not three
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does – lines 96 and 98 go together, not 95 and 98.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it. Sorry. Could you maybe change this to something like this:
serializedValue =
if isObject value
"\n#{ indentLines serializedValue }"
else
" #{ serializedValue }"
That way it's a bit easier to scan the left side of the code and grok the structure. Partially a question of personal taste but I'm fairly certain that matches our (informal) code style.
Wow, that was quick! One small indentation thing but other than that LGTM. |
serializedValue | ||
items = arr.map (value) -> visitNode value, bracesRequired: true | ||
|
||
serializedItems = if indent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - I'd prefer if we keep if
and else
tokens on one height.
@jkrems all comments addressed |
v1.1.0 - sorry for the delay, busy week. |
No worries. Glad to see this merged. Thanks! |
So it looks like this changed the following case: require('cson-parser').stringify({a:{}}, null, 2) Beforea: {} Aftera:
{} Was this intentional? It looks kind of weird to see the empty |
@kevinsawicki Sorry for the regressions, fixed in v1.1.1 thanks to @charlierudolph. |
fixes #36
@jkrems @johan