Conversation
| } | ||
|
|
||
| public static ShaderDescription VertexShaderDynamoMeshDescription = new ShaderDescription(nameof(VertexShaderDynamoMeshDescription), ShaderStage.Vertex, | ||
| public static readonly ShaderDescription VertexShaderDynamoMeshDescription = new ShaderDescription(nameof(VertexShaderDynamoMeshDescription), ShaderStage.Vertex, |
There was a problem hiding this comment.
API breaking, but team agreed on Helix related work
| } | ||
|
|
||
| public static ShaderDescription PixelShaderDynamoMeshDescription = new ShaderDescription(nameof(PixelShaderDynamoMeshDescription), ShaderStage.Pixel, | ||
| public static readonly ShaderDescription PixelShaderDynamoMeshDescription = new ShaderDescription(nameof(PixelShaderDynamoMeshDescription), ShaderStage.Pixel, |
| { | ||
| geometryModels = Element3DDictionary | ||
| .Where(x => x.Key.Contains(node.AstIdentifierGuid) && x.Value as Element3D != null).ToArray(); | ||
| .Where(x => x.Key.Contains(node.AstIdentifierGuid) && x.Value is Element3D).ToArray(); |
There was a problem hiding this comment.
SQ has been back and forth about this, which made me think we may not need this type check at all
| if (!pkgResponse.success) | ||
| { | ||
| throw new Exception(pkgResponse.message); | ||
| throw new ApplicationException(pkgResponse.message); |
There was a problem hiding this comment.
Please, no more System Exception thrown by user code :)
There was a problem hiding this comment.
@QilongTang I know this has been merged already, but as a future reference, ApplicationException is not recommended to be used anymore. From https://docs.microsoft.com/en-us/dotnet/api/system.applicationexception?view=netcore-3.1:
You should derive custom exceptions from the Exception class rather than the ApplicationException class. You should not throw an ApplicationException exception in your code, and you should not catch an ApplicationException exception unless you intend to re-throw the original exception.
I think in this particular case, the most appropriate fix would be to throw a PackageManagerException which derives from Exception.
| /// <returns> true if the element was found </returns> | ||
| public static bool GetFirstNonArrayStackValue(StackValue svArray, ref StackValue sv, RuntimeCore runtimeCore) | ||
| { | ||
| RuntimeMemory rmem = runtimeCore.RuntimeMemory; |
There was a problem hiding this comment.
Unused local var
| protected PythonNodeBase(IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPorts) : base(inPorts, outPorts) | ||
| { | ||
| ArgumentLacing = LacingStrategy.Disabled; | ||
| } |
There was a problem hiding this comment.
Constructors should be grouped near
| public void GettingNodeNameDoesNotTriggerPropertyChangeCycle() | ||
| { | ||
| //add a node | ||
| var numNode = new CoreNodeModels.Input.DoubleInput { X = 100, Y = 100 }; |
There was a problem hiding this comment.
ModeBase.X is obsolete, seems here we do not really need positions to be set
| /// </summary> | ||
| /// <param name="packageDownloadHandle">package download handle</param> | ||
| /// <param name="downloadPath">package download path</param> | ||
| internal void InstallStateCheck(PackageDownloadHandle packageDownloadHandle, string downloadPath) |
There was a problem hiding this comment.
Minor thing: As we are assigning the package state in this method, may be we can call it something like SetPackageState.
|
Looks good to me @QilongTang. |
|
@reddyashish Addressed your comments. Merging. |
3 participants
Please Note:
DynamoRevitrepo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after aLGTMlabel is added to the PR.Purpose
Fix some new code smells brought in last week..
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@DynamoDS/dynamo
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of