Skip to content
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

Add support to experimental features #7242

Merged
merged 17 commits into from Jul 12, 2018

Conversation

@daxian-dbw
Copy link
Member

commented Jul 6, 2018

PR Summary

RFC: PowerShell/PowerShell-RFC#114

Goals:

  • Allow experimental features to be declared by PowerShell engine and modules.
  • Allow experimental features to be enabled via powershell.config.json
  • Allow Function, Cmdlet, parameters and parameter sets to be shown to the user or hiden from the user depending on whether the associated experimetnal feature is on or off.
  • Allow discover experimental features using cmdlet Get-ExperimentalFeature

No Goals:

  • Allow declaring and handling of experimental feature dependencies.
  • Show or hide PowerShell class depending on an experimental is on or off.

Engine Experimental Feature

Engine experimental features are required to register to the ExperimentalFeature type, by adding an ExperimentalFeature instance to ExperimentalFeature.EngineExperimentalFeatures in the type initailizer.

Module Experimental Feature

Module experimetnal features can declared by adding ExperimentalFeatures = @(..) to the PSData section of PrivateData in the module manifest. Example:

PrivateData = @{
    PSData = @{
        ExperimentalFeatures = @(
            @{ Name = 'ExpTest.FeatureOne'; Description = "Test feature number one." }
            @{ Name = 'ExpTest.FeatureTwo'; Description = "Test feature number two." }
        )
    }
}

Enable Experimental Features

Experimental features are enabled by declaring them in the powershell.config.json file. The enabled experimetnal features are fixed after powershell starts, and cannot be changed at run time.

{
  "Microsoft.PowerShell:ExecutionPolicy":"RemoteSigned",
  "ExperimentalFeatures": [
    "PSFileSystemProviderV2"
    "ExpTest.FeatureOne"
  ]
}

Exposed Public APIs

  • System.Management.Automation.ExperimentalAttribute. An attribute that can be applied to types or properties or fields. The attribute is used to support showing or hiding a command (Function, Cmdlet), or a command parameter.
  • A new constructor for ParameterAttribute: Parameter(string experimentalFeatureName, ExperimentAction). This constructor is used to support showing or hiding a parameter set.
  • System.Management.Automation.ExperimentAction. An enum with 3 members: "None", "Show", "Hide". This enum is used when creating an instance of ExperimentalAttribute, or ParameterAttribute with the new constructor. It indicate the action to take for a command or parameter when the associated feature is on or off.
  • System.Management.Automation.ExperimentalFeature. It represents an experimental feature, and it also exposes a static method bool HasEnabled(string featureName) for users to check if a given experimental feature is on.

New Type Accelerators

  • ExperimentAction for System.Management.Automation.ExperimentAction
  • Experimental for System.Management.Automation.ExperimentalAttribute
  • ExperimentalFeature for System.Management.Automation.ExperimentalFeature

New Cmdlet

Add Get-ExperimentalFeature [[-Name] <string[]>] [-ListAvailable] [<CommonParameters>] to retrieve information about experimental features from engine or modules. Name takes wildcards. When -ListAvailable is not specified, Get-ExperimentalFeature only returns the enabled experimental features (mimic Get-Module). When -ListAvailable is specified, Get-ExperimentalFeature returns all available experimental features from engine and modules in module paths.

PR Checklist

@@ -1758,8 +1758,10 @@ private List<CompletionResult> GetResultForAttributeArgument(CompletionContext c
List<CompletionResult> result = new List<CompletionResult>();
foreach (PropertyInfo pro in propertyInfos)
{
//Ignore TypeId (all attributes inherit it)
if (pro.Name != "TypeId" && (pro.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase)))

This comment has been minimized.

Copy link
@TravisEz13

TravisEz13 Jul 7, 2018

Member

why did the != "TypeId go away?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 9, 2018

Author Member

The property TypeId is a read-only property. This change adds two more read-only properties to ParameterAttribute: ExperimentName and ExperimentAction. They shouldn't show up in the tab completion of Attribute properties because you cannot set values for them. So we should just ignore all getter properties.

}
private ExperimentAction _effectiveAction = default(ExperimentAction);
}
}

This comment has been minimized.

Copy link
@TravisEz13

TravisEz13 Jul 7, 2018

Member

missing newline

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 9, 2018

Author Member

Fixed.

}
}
}
}

This comment has been minimized.

Copy link
@TravisEz13

TravisEz13 Jul 7, 2018

Member

Newline?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Fixed.

}
else
{
s_PSSnapInTracer.WriteLine("Executing IModuleAssemblyInitializer.Import for {0}", assemblyPath);

This comment has been minimized.

Copy link
@TravisEz13

TravisEz13 Jul 7, 2018

Member

seems like the snapin related changes would be better in a different PR

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 9, 2018

Author Member

Actually, good catch here!
I reviewed my changes here again and this if...else... block shouldn't be removed. We should run ModuleInitializer when importing a binary module, even if the module was already loaded through an earlier import. I will revert this change.

}

Array.Sort(featureNames);
string allNames = String.Join(Environment.NewLine, featureNames);

This comment has been minimized.

Copy link
@TravisEz13

TravisEz13 Jul 7, 2018

Member

maybe it's better to join with a static value instead of NewLine? Is NewLine static or based on culture?

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 7, 2018

Member

I think newline is platform based: \n vs \r\n

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 8, 2018

Member

I looked into it, Environment.NewLine is compile-time static (the way corefx does it is they have a partial class with the shared part and then a .Unix.cs and a .Windows.cs, etc. and then they conditionally include it in compilation).

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 9, 2018

Author Member

Yes, Environment.NewLine is static, \r\n on Windows and \n on Unix platforms. Here we are calculating a hash that is used locally for the analysis cache file name, so it's OK as long as it's consistent on a specific platform.

@rjmholt rjmholt requested a review from TylerLeonhardt Jul 7, 2018

@@ -81,6 +81,16 @@
InnerException: {3}
</value>
</data>
<data name="PS_PROVIDEReventE_O_ExperimentalFeatureInvalidName" xml:space="preserve">
<value>Experimental Feature Initialization: Ignore the experimental feature '{0}' from the config file. {1}</value>

This comment has been minimized.

Copy link
@TravisEz13

TravisEz13 Jul 7, 2018

Member

seems on that the message doesn't say the name is invalid.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 9, 2018

Author Member

The {1} part will say why this name is invalid.

/// <summary>
/// Search module paths to find all available experimental features.
/// </summary>
[Parameter()]

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 7, 2018

Member

Parentheses are used here but not above in a no-arg attribute -- should probably be consistent

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Fixed.

@daxian-dbw daxian-dbw changed the title [WIP] Add support to experimental features Add support to experimental features Jul 10, 2018

yield return new FormatViewDefinition("ExperimentalFeature",
TableControl.Create()
.AddHeader(Alignment.Left, width: 35)
.AddHeader(Alignment.Right, width: 10)

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Why is the Enabled column width 10? Why not 7 (length of Enabled)?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Good point. Fixed.

{
yield return new FormatViewDefinition("ExperimentalFeature",
TableControl.Create()
.AddHeader(Alignment.Left, width: 35)

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Seems like Names should be terse (and unique), so 35 seems quite wide.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

When it's a module feature (feature declared in module), the feature name would be longer than the module name. The width for module name is 35, and I also use 35 here for the feature name. If you still think we should use a smaller size, I can make that change.

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Ok, for a module it makes sense. Let's leave it at 35 for now. Formatting changes aren't considered breaking so we can adjust later.

TableControl.Create()
.AddHeader(Alignment.Left, width: 35)
.AddHeader(Alignment.Right, width: 10)
.AddHeader(Alignment.Left, width: 35)

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

35 here also seems wide for the Source?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

For a module feature, the source is the path to the module manifest file. That's why I also make the width 35. Let me know if you think we should use a smaller size.

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Ok for now.


using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Collections.Immutable;

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

I comes before O. You almost have these in alphabetical order

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Fixed.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

I don't see the fix.

int firstDotIndex = featureName.IndexOf('.');
int lastDotIndex = featureName.LastIndexOf('.');

bool legit = firstDotIndex > 0 && lastDotIndex < featureName.Length - 1;

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Is this more performant than just

bool legit = !featureName.StartsWith(".") && !featureName.EndsWith(".")

? Which seems a bit more descriptive.

object privateData = null;
if (data.Contains("PrivateData"))
// Set the private data member for the module if the manifest contains this member
object privateData = data["PrivateData"];

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

PrivateDate and PSData should be members of SpecialVariables

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

But they are not variables ... They are module manifest keys.
We could have a static type called ModuleManifestKeys, where we can define all manifest key const strings just like SpecialVariables. But we can do it in a separate PR.

$AssemblyPath = Join-Path $TestModule "ExpTest.dll"
if (-not (Test-Path $AssemblyPath)) {
## When using $SourcePath directly, 'Add-Type' fails in AppVeyor CI runs with an 'access denied' error.
## It turns out Pester doesn't handle an exception like this from 'BeforeAll'. It causes the Pester to

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Did we open an issue in Pester repo or does one already exist?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Didn't open an issue yet. Will do.

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Can you add link to bug in comment? Don't want future tests following this pattern if it gets fixed in Pester.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Will do after I open the issue.

param($Name, $CommandType)
$command = Get-Command $Name
$command.CommandType | Should -Be $CommandType
## 11 common parameters + '-Name'

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

For future maintenance of this code, it would be better to have something like:

BeforeAll {
  $commonParameters = @('a','b','c',...)
}
...
$command.Parameters.Count | Should -Be ($commonParameters + "-Name").Length

Apply in all cases below. I think it removes the need for the comments.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

I should have used [System.Management.Automation.Internal.CommonParameters].GetProperties().Length to get the count of common parameters. I will make the change.

}
}

#endregion

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

You're missing a test cases for:

  1. [Experimental()]
  2. ExperimentAction.None
  3. ExperimentName starting and ending with period

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Tests added.

#region Experimental Feature Related Properties

/// <summary>
/// Name of the experimental feature this attribute is associated with.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Should be: "Gets name of the experimental feature this attribute is associated with."

public string ExperimentName { get; }

/// <summary>
/// Action for engine to take when the experimental feature is enabled.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Should be "Gets action for engine to take when the experimental feature is enabled."

internal bool ToShow => EffectiveAction == ExperimentAction.Show;

/// <summary>
/// Effective action to take at run time.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Should be "Gets effective action to take at run time.".

//Ignore TypeId (all attributes inherit it)
if (pro.Name != "TypeId" && (pro.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase)))
// Ignore getter-only properties, including 'TypeId' (all attributes inherit it).
if (!property.CanWrite) { continue; }

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Seems our convention is

if (!property.CanWrite) continue;

or

if (!property.CanWrite)
{
    continue;
}

I'd prefer second.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

We use if () { // simple statement } all over the code base. But I don't think we should allow the first pattern in your comment. The if body should always be in { }.

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 11, 2018

Member

Certainly agree that we should always use braces.

In a world where we no longer use teletypes (i.e. where screens are big and newlines are cheap), I tend to feel that we should always just hang the line so that as my eyes scan down a document at an approximate indentation level and always know what's going on. But, that's just a style preference, and inline-braces are still pretty good to me.


using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Collections.Immutable;

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

I don't see the fix.

}

// If all 'ParameterAttribute' declared for the parameter are hidden due to
// an experimental feature, then the parameter should be ignored.
if (hasParameterAttribute && !hasEnabledParamAttribute) { return null; }

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Please use multiline formatting.

attributes.Reverse();
if (!hasParameterAttribute)
{
attributes.Insert(0, new ParameterAttribute());
}

var result = new RuntimeDefinedParameter(parameterAst.Name.VariablePath.UserPath, parameterAst.StaticType,
new Collection<Attribute>(attributes.ToArray()));
new Collection<Attribute>(attributes));

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Formatting - please place one parameter per line.

{
var attributeAst = ParamBlock.Attributes[index];
var expAttr = GetExpAttributeHelper(attributeAst);
if (expAttr != null) { yield return expAttr; }

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Please use multiline formatting.

{
return Compiler.GetAttribute(potentialExpAttr) as ExperimentalAttribute;
}
catch (Exception) { /* catch all and assume it's not a declaration of ExperimentalAttribute */ }

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

I guess we get style issues in CodeFactor for the formatting.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

I will update this one as it's not exactly an empty catch body.

@@ -117,6 +117,9 @@
<resheader name="writer">
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
</resheader>
<data name="EnabledExperimentalFeatures" xml:space="preserve">
<value>Variable to hold the enabled experimental feature names</value>

This comment has been minimized.

Copy link
@iSazonov

iSazonov Jul 10, 2018

Collaborator

Should we add final dot?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

I followed the existing examples. I don't have a strong opinion. It looks OK to me without the dot:

PS:8> $v.Description
Parent folder of the host application of the current runspace
}

It "Argument validation for constructors of 'ExperimentalAttribute' and 'ParameterAttribute'" {
{ [Experimental]::new("", "None") } | Should -Throw -ErrorId "PSArgumentNullException"

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Jul 10, 2018

Member

Would be better to have these as TestCases so that the first failure doesn't prevent the rest from running

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 10, 2018

Author Member

Will update the test.

@daxian-dbw

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2018

@iSazonov About the style rules, @TravisEz13, @adityapatwardhan and I went through all issues reported from CodeFactor a few weeks back, and we got a list of rules that we think should be turned off. Here is the list:
1305: SA1305FieldNamesMustNotUseHungarianNotation
1502: SA1502ElementMustNotBeOnSingleLine
1310: SA1310FieldNamesMustNotContainUnderscore,
1204: SA1204StaticElementsMustAppearBeforeInstanceElements
1009: SA1009ClosingParenthesisMustBeSpacedCorrectly
1501:SA1501StatementMustNotBeOnSingleLine
1513:SA1513ClosingBraceMustBeFollowedByBlankLine
1306:SA1306FieldNamesMustBeginWithLowerCaseLetter
1308:SA1308VariableNamesMustNotBePrefixed
1013:SA1013ClosingBracesMustBeSpacedCorrectly
1500:SA1500BracesForMultiLineStatementsMustNotShareLine
1010:SA1010OpeningSquareBracketsMustBeSpacedCorrectly
1026:SA1026CodeMustNotContainSpaceAfterNewKeywordInImplicitlyTypedArrayAllocation
1008:SA1008OpeningParenthesisMustBeSpacedCorrectly
1311:SA1311StaticReadonlyFieldsMustBeginWithUpperCaseLetter
1025:SA1025CodeMustNotContainMultipleWhitespaceInARow
1012:SA1012OpeningBracesMustBeSpacedCorrectly
1215:SA1215InstanceReadonlyElementsMustAppearBeforeInstanceNonReadonlyElements
1214:SA1214ReadonlyElementsMustAppearBeforeNonReadonlyElements
1210:SA1210UsingDirectivesMustBeOrderedAlphabeticallyByNamespace
1609:SA1609PropertyDocumentationMustHaveValue

The rules about "one parameter per line" for method definition or invocation is not in this list, but in my opinion, we probably should disable that rule because 1) it doesn't bring much benefit -- as long as the parameters are well aligned, there is no readability issue; 2) there are too many instances in our existing code base that are violating that rule, and fixing all doesn't worth the time.

The following document rules make sense for public members, but they are applied to non-public members too, which causes a lot of noise data.

1611:SA1611ElementParametersMustBeDocumented
1615:SA1615ElementReturnValueMustBeDocumented
1606:SA1606ElementDocumentationMustHaveSummaryText

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2018

@daxian-dbw I moved your style comment in Issue #4708.

@daxian-dbw

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2018

@iSazonov Thanks for moving it to the right place. I will spend more time refining the rules for CodeFactor after finishing the first check-in of the experimental feature work.

//Ignore TypeId (all attributes inherit it)
if (pro.Name != "TypeId" && (pro.Name.StartsWith(argName, StringComparison.OrdinalIgnoreCase)))
// Ignore getter-only properties, including 'TypeId' (all attributes inherit it).
if (!property.CanWrite) { continue; }

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 11, 2018

Member

Certainly agree that we should always use braces.

In a world where we no longer use teletypes (i.e. where screens are big and newlines are cheap), I tend to feel that we should always just hang the line so that as my eyes scan down a document at an approximate indentation level and always know what's going on. But, that's just a style preference, and inline-braces are still pretty good to me.

s_cacheStoreLocation =
Environment.GetEnvironmentVariable("PSModuleAnalysisCachePath") ??
// If user defines a custom cache path, then use that.
string userDefinedCachePath = Environment.GetEnvironmentVariable("PSModuleAnalysisCachePath");

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 11, 2018

Member

I would say it does constitute a public API, so we ought to document it, although there's not really much need

/// Engine features come before module features.
/// Within engine features and more features, features are ordered by name.
/// </remarks>
private static string GetSortingString(ExperimentalFeature feature)

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 11, 2018

Member

@daxian-dbw This is the only thing remaining I can think of. May not be significant, but returning (int, string) here might save some allocations

/// </remarks>
private static string GetSortingString(ExperimentalFeature feature)
{
if (ExperimentalFeature.EngineSource.Equals(feature.Source))

This comment has been minimized.

Copy link
@rjmholt

rjmholt Jul 11, 2018

Member

Does this need to be case insensitive?

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 12, 2018

Author Member

I was assuming people will use the const string field ExperimentalFeature.EngineSource for registering an engine exp feature, but it's more reliable to make this comparison case insensitive.

@daxian-dbw

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

@TravisEz13 is on vacation. I assigned this PR to @adityapatwardhan

@adityapatwardhan
Copy link
Member

left a comment

Minor nitpick. Looks good otherwise.

/>
<string
id="PS_PROVIDER.event.E_O_ExperimentalFeatureReadConfigError.message"
value="Experimental Feature Initialization: Failed to read the config file.%n Exception: %1 %n Message: %2 %n StackTrace: %3 %n"

This comment has been minimized.

Copy link
@adityapatwardhan

adityapatwardhan Jul 12, 2018

Member

Is there a space needed between file. and %n

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Jul 12, 2018

Author Member

%n is a new line, so no space is needed here. For Exception: %1 %n Message: %2 %n StackTrace: %3 %n, the space character before %n is not necessary, but it's been that way in other places of this manifest file, so I followed the existing pattern.

@adityapatwardhan adityapatwardhan merged commit 75ba74c into PowerShell:master Jul 12, 2018

4 of 5 checks passed

CodeFactor 52 issues fixed. 184 issues found.
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@daxian-dbw daxian-dbw deleted the daxian-dbw:exp branch Jul 12, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jul 13, 2018

@daxian-dbw Congratulations on this great work! 👍

TravisEz13 added a commit that referenced this pull request Aug 14, 2018

Fix some style issues in engine code (#7246)
The PR resolves some style issues in engine code. (Moved from PR #7242. )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.