Resolving type/command/event name clashes #1222

paulbalaji · 2019-11-19T17:12:01Z

Description

Potential name clash between nested types and fields in schema components #958 (UTY-1416)
- codegen will now catch this, skip the field, and log an error
Potential name clash between components and events/commands in generated code #957 (UTY-1962)
- codegen will now catch this, skip the event/command, and log an error

It currently just does a basic log to stderr, and forwarding that to the Editor:

TODO:

changelog
move valid sample schema to test project

~~add tests for the illegal schema sample~~

UTY-2311 will implement logging in the codegen, which should provider a nicer way of outputting the information/errors that we've added in this PR. For now, how it currently logs can be left as an implementation detail.

Tests

Schema linked in #957 and #958 will no longer cause compilation errors, but codegen will log an error that gets picked up in the Editor

Primary reviewers

If your change will take a long time to review, you can name at most two primary reviewers who are ultimately responsible for reviewing this request. @ mention them.

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

…ad of erroring out

…uld no longer fail

improbable-prow-robot · 2019-11-19T17:12:03Z

Corresponding JIRA ticket: https://improbableio.atlassian.net/browse/UTY-1416

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

jamiebrynes7 · 2019-11-19T23:15:39Z

...probable.gdk.tools/.CodeGenTemplate/CodeGenerationLib/Model/Details/UnityComponentDetails.cs

+                isValid = false;
+                Console.Error.WriteLine(
+                    $"Error in component \"{componentName}\". " +
+                    $"Command \"{clashingCommand.RawCommandName}\" clashes with component name.");


Nit: Prefer having a long line over concatentating strings. Also you can use single quotes and get rid of the escaping which aids readability

I was using single quotes before and I think it's not really that readable in the Editor

...probable.gdk.tools/.CodeGenTemplate/CodeGenerationLib/Model/Details/UnityComponentDetails.cs

...io.improbable.gdk.tools/.CodeGenTemplate/CodeGenerationLib/Model/Details/UnityTypeDetails.cs

...ges/io.improbable.gdk.tools/.CodeGenTemplate/CodeGenerationLib/Model/Details/DetailsStore.cs

jamiebrynes7 · 2019-11-19T23:23:07Z

test-project/Assets/.schema/illegal.schema

@@ -0,0 +1,27 @@
+// Wait, that's illegal!


...ges/io.improbable.gdk.tools/.CodeGenTemplate/CodeGenerationLib/Model/Details/DetailsStore.cs

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

zeroZshadow

I think codegen needs to fail when running into clashes, not skip them.

zeroZshadow · 2019-11-21T11:12:08Z

...probable.gdk.tools/.CodeGenTemplate/CodeGenerationLib/Model/Details/UnityComponentDetails.cs

@@ -12,8 +13,8 @@ public class UnityComponentDetails
        public bool IsBlittable { get; }

        public IReadOnlyList<UnityFieldDetails> FieldDetails { get; private set; }
-        public IReadOnlyList<UnityCommandDetails> CommandDetails { get; }
-        public IReadOnlyList<UnityEventDetails> EventDetails { get; }
+        public IReadOnlyList<UnityCommandDetails> CommandDetails { get; private set; }


Why are these 2 private set now?

nice spot - these are relics from when the filtering was done in a separate method

zeroZshadow · 2019-11-21T11:18:59Z

...probable.gdk.tools/.CodeGenTemplate/CodeGenerationLib/Model/Details/UnityComponentDetails.cs

+                        return true;
+                    }
+
+                    Console.Error.WriteLine($"Error in component \"{ComponentName}\". Command \"{commandDetail.RawCommandName}\" clashes with component name.");


This should FAIL codegen. It shouldn't be possible to write schema that throws errors but still generates code.

resolved offline

jamiebrynes7 · 2019-11-21T11:29:06Z

I think codegen needs to fail when running into clashes, not skip them.

The problem is until we have a way of surfacing these errors reliably, failing code generation (i.e. - outputting nothing) obscures the problem due to the avalanche of compile errors you will hit.

paulbalaji · 2019-11-21T13:53:46Z

I think codegen needs to fail when running into clashes, not skip them.

The problem is until we have a way of surfacing these errors reliably, failing code generation (i.e. - outputting nothing) obscures the problem due to the avalanche of compile errors you will hit.

Discussed offline

Having heard both sides, I've decided to keep the messages as errors without failing the entire codegen step for now, and we can refine the idea further when we have better codegen logging. Once that is in place, we are likely to be able to surface these issues better to users and (maybe) cause the codegen step to fail in a way that doesn't stop users from A) knowing that the error was in codegen and B) knowing where to look to figure out where their error comes from.

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

Paul Balaji added 4 commits November 19, 2019 14:54

first pass

a1eae6b

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

another pass to resolve event/command <> component name clashes inste…

0c46c7c

…ad of erroring out

[TO MOVE TO TEST PROJECT LATER] add schema that used to fail that sho…

e3f952b

…uld no longer fail

fix ordering

28e8eb6

improbable-prow-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/UTY size/L Denotes a PR that changes 150-299 lines, ignoring generated files. labels Nov 19, 2019

paulbalaji requested review from jamiebrynes7, zeroZshadow and jessicafalk November 19, 2019 17:12

improbable-prow-robot added A: playground Area: Playground A: tooling Area: Tooling labels Nov 19, 2019

no

4756262

improbable-prow-robot added size/M Denotes a PR that changes 40-149 lines, ignoring generated files. and removed size/L Denotes a PR that changes 150-299 lines, ignoring generated files. labels Nov 19, 2019

Paul Balaji added 2 commits November 19, 2019 18:21

Wait, that's illegal!

2acab6d

changelog

179e4f6

paulbalaji marked this pull request as ready for review November 19, 2019 18:27

improbable-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 19, 2019

Paul Balaji added 3 commits November 19, 2019 18:27

Merge branch 'develop' into bugfix/uty-1416

be69591

too illegal for my own good

dbc788e

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

too illegal for my own good

03b4a8d

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

jamiebrynes7 reviewed Nov 19, 2019

View reviewed changes

Paul Balaji added 3 commits November 20, 2019 14:38

don't remove entire type/components now

1627842

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

fix changelog

3e2df08

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

minutae

0f4117a

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

paulbalaji requested a review from jamiebrynes7 November 20, 2019 15:01

jamiebrynes7 approved these changes Nov 20, 2019

View reviewed changes

zeroZshadow suggested changes Nov 21, 2019

View reviewed changes

no set, only get

f46ad9a

Signed-off-by: Paul Balaji <paulbalaji@improbable.io>

paulbalaji requested a review from zeroZshadow November 21, 2019 13:59

zeroZshadow approved these changes Nov 21, 2019

View reviewed changes

jessicafalk approved these changes Nov 21, 2019

View reviewed changes

paulbalaji merged commit 5385d3c into develop Nov 21, 2019

paulbalaji deleted the bugfix/uty-1416 branch November 21, 2019 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolving type/command/event name clashes #1222

Resolving type/command/event name clashes #1222

paulbalaji commented Nov 19, 2019 •

edited

Loading

improbable-prow-robot commented Nov 19, 2019

jamiebrynes7 Nov 19, 2019

paulbalaji Nov 20, 2019

jamiebrynes7 Nov 19, 2019

zeroZshadow left a comment

zeroZshadow Nov 21, 2019

paulbalaji Nov 21, 2019

zeroZshadow Nov 21, 2019

paulbalaji Nov 21, 2019

jamiebrynes7 commented Nov 21, 2019

paulbalaji commented Nov 21, 2019

Resolving type/command/event name clashes #1222

Resolving type/command/event name clashes #1222

Conversation

paulbalaji commented Nov 19, 2019 • edited Loading

Description

Tests

Primary reviewers

improbable-prow-robot commented Nov 19, 2019

jamiebrynes7 Nov 19, 2019

Choose a reason for hiding this comment

paulbalaji Nov 20, 2019

Choose a reason for hiding this comment

jamiebrynes7 Nov 19, 2019

Choose a reason for hiding this comment

zeroZshadow left a comment

Choose a reason for hiding this comment

zeroZshadow Nov 21, 2019

Choose a reason for hiding this comment

paulbalaji Nov 21, 2019

Choose a reason for hiding this comment

zeroZshadow Nov 21, 2019

Choose a reason for hiding this comment

paulbalaji Nov 21, 2019

Choose a reason for hiding this comment

jamiebrynes7 commented Nov 21, 2019

paulbalaji commented Nov 21, 2019

paulbalaji commented Nov 19, 2019 •

edited

Loading