Skip to content
This repository has been archived by the owner on Jan 18, 2022. It is now read-only.

Feature/schema compiler error message #1107

Merged
merged 6 commits into from
Aug 12, 2019

Conversation

JayImprobable
Copy link
Contributor

@JayImprobable JayImprobable commented Aug 9, 2019

Description

Originally, the RedirectedProcess responsible for compiling the schemas was set as

var exitCode = RedirectedProcess.Command(Common.DotNetBinary)
                        .WithArgs(ConstructArgs(projectPath, schemaCompilerPath, workerJsonPath))
                        .RedirectOutputOptions(OutputRedirectBehaviour.None)
                        .AddErrorProcessing(Debug.LogError)
                        .AddOutputProcessing(ProcessStdOut)
                        .Run();

This way, whenever a new error message was generated, Debug.LogError was called, creating several error messages on the Console.
What I did was:

  • In the GenerateCode class I've created a private static string SchemaCompilingErrorMessage variable, where I store the complete error message.
        private static void SchemaCompilerErrorProcessing(string s)
        {
            SchemaCompilingErrorMessage = $"{SchemaCompilingErrorMessage}\n{s}";
        }
  • Changed the RedirectedProcess to
var exitCode = RedirectedProcess.Command(Common.DotNetBinary)
                        .WithArgs(ConstructArgs(projectPath, schemaCompilerPath, workerJsonPath))
                        .RedirectOutputOptions(OutputRedirectBehaviour.None)
                        .AddErrorProcessing(SchemaCompilerErrorProcessing)
                        .AddOutputProcessing(ProcessStdOut)
                        .Run();
  • Created the SchemaCompilerErrorProcessing function, which simply concatenates the error messages into the SchemaCompilingErrorMessage

  • And finally, if there was any errors in the schema compiling, I call Debug.LogError(SchemaCompilingErrorMessage);, showing the single, concatenated error message

Tests

Tested it manually by adding errors to .schema files

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.

Tests

How did you test these changes prior to submitting this pull request?
What automated tests are included in this PR?

Documentation

How is this documented (for example: release note, upgrade guide, feature page, in-code documentation)?

Did you remember a changelog entry?

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.

@improbable-prow-robot improbable-prow-robot added jira/no-ticket Indicates a PR has no corresponding JIRA ticket A: tooling Area: Tooling size/XS Denotes a PR that changes 0-14 lines, ignoring generated files. labels Aug 9, 2019
CHANGELOG.md Outdated Show resolved Hide resolved
Changed description in changelog and removed static global variable
Copy link
Contributor

@jamiebrynes7 jamiebrynes7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % nit

@JayImprobable JayImprobable merged commit e35d48b into develop Aug 12, 2019
@jared-improbable jared-improbable deleted the feature/schema_compiler_error_message branch August 12, 2019 11:50
jessicafalk pushed a commit that referenced this pull request Nov 15, 2019
…cenarios (#1107)

* Refactor coordinator worker to add support for Regisseur scenarios

* Convert tabs to spaces

* Update SpatialGDK/Build/Programs/Improbable.Unreal.Scripts/WorkerCoordinator/Program.cs

Co-Authored-By: girayimprobable <[email protected]>

* Remove redundant Split
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A: tooling Area: Tooling jira/no-ticket Indicates a PR has no corresponding JIRA ticket size/XS Denotes a PR that changes 0-14 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants