Fix line wrapping in json_schema_compiler

Fix line wrapping in json_schema_compiler

BUG=563744
Committed: https://crrev.com/74d53a0ec7c2be54f4a55a33a66904f1405b947d
Cr-Commit-Position: refs/heads/master@{#370540}

[stevenjb, 2015-12-11 18:49:26]

stevenjb@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[stevenjb, 2015-12-11 18:49:27]

I decided to dodge this issue when I noticed some bad wrapping in the interface
test (by shortening an enum name), but ran into it again for the
networking_private interface and decided to fix it correctly.

[Devlin, 2015-12-15 21:19:29]

I preferred being able to return a code object in these places, rather than
passing in one to be expanded - especially since now we do both.  Can we really
not make this work while maintaining more of a builder pattern?

[stevenjb, 2015-12-15 22:45:10]

I tried that approach originally but it breaks assumptions in code.py and I
wanted to avoid significant changes to that module. The code already seemed
inconsistent to me wrt when a new Code() instance is created. Now it should
only be doing that for single line blocks (and at the top level of course).

If you would like to try you hand at modifying code.py to support line
wrapping for indented code blocks, I would be happy to review it... :)


On Tue, Dec 15, 2015 at 1:19 PM, <rdevlin.cronin@chromium.org> wrote:

> I preferred being able to return a code object in these places, rather than
> passing in one to be expanded - especially since now we do both.  Can we
> really
> not make this work while maintaining more of a builder pattern?
>
> https://codereview.chromium.org/1513573012/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Devlin, 2015-12-18 22:32:37]

On 2015/12/15 22:45:10, stevenjb wrote:
> I tried that approach originally but it breaks assumptions in code.py and I
> wanted to avoid significant changes to that module. The code already seemed
> inconsistent to me wrt when a new Code() instance is created. Now it should
> only be doing that for single line blocks (and at the top level of course).
> 
> If you would like to try you hand at modifying code.py to support line
> wrapping for indented code blocks, I would be happy to review it... :)

I'm still thinking we might be able to get this to work right. ;)

Can you upload add and upload a test that would previously break to this CL,
since we couldn't find an example just cruising around the source?

[stevenjb, 2015-12-18 23:08:40]

python ./tools/json_schema_compiler/compiler.py -g interface
extensions/common/api/bluetooth.idl >
third_party/closure_compiler/externs/bluetooth_interface.js


On Fri, Dec 18, 2015 at 2:32 PM, <rdevlin.cronin@chromium.org> wrote:

> On 2015/12/15 22:45:10, stevenjb wrote:
>
>> I tried that approach originally but it breaks assumptions in code.py and
>> I
>> wanted to avoid significant changes to that module. The code already
>> seemed
>> inconsistent to me wrt when a new Code() instance is created. Now it
>> should
>> only be doing that for single line blocks (and at the top level of
>> course).
>>
>
> If you would like to try you hand at modifying code.py to support line
>> wrapping for indented code blocks, I would be happy to review it... :)
>>
>
> I'm still thinking we might be able to get this to work right. ;)
>
> Can you upload add and upload a test that would previously break to this
> CL,
> since we couldn't find an example just cruising around the source?
>
> https://codereview.chromium.org/1513573012/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2016-01-19 18:45:31]

On 2015/12/18 23:08:40, stevenjb wrote:
> python ./tools/json_schema_compiler/compiler.py -g interface
> extensions/common/api/bluetooth.idl >
> third_party/closure_compiler/externs/bluetooth_interface.js
> 
> 
> On Fri, Dec 18, 2015 at 2:32 PM, <mailto:rdevlin.cronin@chromium.org> wrote:
> 
> > On 2015/12/15 22:45:10, stevenjb wrote:
> >
> >> I tried that approach originally but it breaks assumptions in code.py and
> >> I
> >> wanted to avoid significant changes to that module. The code already
> >> seemed
> >> inconsistent to me wrt when a new Code() instance is created. Now it
> >> should
> >> only be doing that for single line blocks (and at the top level of
> >> course).
> >>
> >
> > If you would like to try you hand at modifying code.py to support line
> >> wrapping for indented code blocks, I would be happy to review it... :)
> >>
> >
> > I'm still thinking we might be able to get this to work right. ;)
> >
> > Can you upload add and upload a test that would previously break to this
> > CL,
> > since we couldn't find an example just cruising around the source?
> >
> > https://codereview.chromium.org/1513573012/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Can we make a call on this? We'd like to change the format for the interfaces,
but I am reluctant to make that change with the current broken line wrapping.

[Devlin, 2016-01-19 21:41:49]

On 2016/01/19 18:45:31, stevenjb wrote:
> Can we make a call on this? We'd like to change the format for the interfaces,
> but I am reluctant to make that change with the current broken line wrapping.

Hacked on this briefly and found the problem - we don't do a good job of
re-adjusting line lengths when concatening one code object onto another.  The
right-est way to fix this would be to have a more powerful Line object in
code.py so that we could defer the formatting until Code.Render(), but that's a
much bigger change (though it would probably make things a lot more logical in
the long run).  Assuming you don't want to do that (which is totally fine - I
don't want to right now either), this approach LGTM.

One request - can we change GenerateFoo to be AppendFoo?

[stevenjb, 2016-01-20 21:48:35]

On 2016/01/19 21:41:49, Devlin wrote:
> On 2016/01/19 18:45:31, stevenjb wrote:
> > Can we make a call on this? We'd like to change the format for the
interfaces,
> > but I am reluctant to make that change with the current broken line
wrapping.
> 
> Hacked on this briefly and found the problem - we don't do a good job of
> re-adjusting line lengths when concatening one code object onto another.  The
> right-est way to fix this would be to have a more powerful Line object in
> code.py so that we could defer the formatting until Code.Render(), but that's
a
> much bigger change (though it would probably make things a lot more logical in
> the long run).  Assuming you don't want to do that (which is totally fine - I
> don't want to right now either), this approach LGTM.
> 
> One request - can we change GenerateFoo to be AppendFoo?

Done.

[stevenjb, 2016-01-20 21:48:41]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2016-01-20 21:48:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1513573012/#ps40001
(title: "Generate -> Append")

[commit-bot: I haz the power, 2016-01-20 21:50:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513573012/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513573012/40001

[commit-bot: I haz the power, 2016-01-21 00:08:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-21 00:13:15]

Description was changed from

==========
Fix line wrapping in json_schema_compiler

BUG=563744
==========

to

==========
Fix line wrapping in json_schema_compiler

BUG=563744

Committed: https://crrev.com/74d53a0ec7c2be54f4a55a33a66904f1405b947d
Cr-Commit-Position: refs/heads/master@{#370540}
==========

[commit-bot: I haz the power, 2016-01-21 00:13:16]

Patchset 3 (id:??) landed as
https://crrev.com/74d53a0ec7c2be54f4a55a33a66904f1405b947d
Cr-Commit-Position: refs/heads/master@{#370540}