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.

Sign up for GitHub

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

Jump to bottom

Fix API breaking changes #14916

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Fix API breaking changes #14916

merged 4 commits into from
Feb 2, 2024

Conversation

QilongTang
Copy link
Contributor

Purpose

Fix API breaking changes introduced at https://github.com/DynamoDS/Dynamo/pull/14871/files#diff-1475bce3e023560596915598089a401f981794dfb28b5855183653a85fbc707fR466

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

N/A

Reviewers

@DynamoDS/dynamo

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@QilongTang QilongTang added this to the 3.1 milestone Jan 31, 2024
mjkkirschner
mjkkirschner reviewed Jan 31, 2024
View reviewed changes
src/DynamoCore/Models/RecordableCommands.cs
/// </summary>
/// <param name="filePath">The path to the file.</param>
/// <param name="forceManualExecutionMode">Should the file be opened in manual execution mode?</param>
public OpenFileCommand(string filePath, bool forceManualExecutionMode = false)
Copy link
Member

Choose a reason for hiding this comment

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

do you want to obsolete this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really since the old one is still pretty useful..

Copy link
Contributor

Choose a reason for hiding this comment

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

The old API was public OpenFileCommand(string filePath, bool forceManualExecutionMode = false)
All 3dParty calls to OpenFileCommand(filepath) would now cause an ambiguous exception;

Copy link
Member

Choose a reason for hiding this comment

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

@pinzart90 have you verified that? I recall that the overload is actually resolved at compile time - but it's been a long time.

Copy link
Member

@mjkkirschner mjkkirschner Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

see this... it's subtle.
https://stackoverflow.com/a/40628542

It's similar to this case, though not exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pinzart90 I think that's exactly why I need to fix all the other cases in this PR, I could make the other constructor OpenFileCommand(string filePath, bool isTemplate, bool forceManualExecutionMode = false) so the original one is not ambiguous, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

It may be ok. We should test with pre compiled binaries (like from revit) that call OpenFileCommand("", false); and OpenFileCommand("") running against these changes.

Copy link
Member

@mjkkirschner mjkkirschner Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

one easy way to do that @pinzart90 @QilongTang is to write a test that is compiled against 2.x and run it against the new dynamo core binaries from this pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

O yeah, nice. We've done that before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have modified the new constructor to be a strong signature so we do not have API breaking problem as public OpenFileCommand(string filePath, bool forceManualExecutionMode, bool isTemplate). Notice that I do not mean to obsolete the old constructor because I dont think the callers need to specify the isTemplate all the time, on the contrary, I think the original constructor should still be most useful.

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 1, 2024 •
edited
Loading

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

@reddyashish
Copy link
Contributor

The EngOps build failed though?

@QilongTang
Copy link
Contributor Author

The EngOps build failed though?

It's failing for all the recent jobs in build step, not sure why

@QilongTang QilongTang merged commit d0bd96e into master Feb 2, 2024
21 of 22 checks passed
@QilongTang QilongTang deleted the APIBreakingChange branch February 2, 2024 20:56
QilongTang pushed a commit that referenced this pull request Feb 6, 2024
@RobertGlobant20
DYN-6535 DynamoRevit Improve Search Fix 2 (#14917)
d01e057 
* DYN-6535 DynamoRevit Improve Search Fix 2

The specific case "element set parameter" was failing on Revit 2025 (not finding anything), then I've modified the code so that check if the first term is a category if that is the case it will create a specific query using the first term as category and the next terms as node name.

* DYN-6535 DynamoRevit Improve Search Fix 2 Code Review

Updating Linq query so that is executed only once.

* Squashed commit of the following:

commit 3893c85
Author: Ashish Aggarwal <[email protected]>
Date:   Mon Feb 5 12:53:49 2024 -0500

    Fix retain package structure flow where nested directory at root level were lost (#14918)

    * optimize

    * comments and a test

    * add subdir test and comments

    * add child test

    * test updates

commit d0bd96e
Author: Aaron (Qilong) <[email protected]>
Date:   Fri Feb 2 15:56:03 2024 -0500

    Fix API breaking changes (#14916)

    * Fix API breaking changes

    * Update OpenFileCommand

    * update

commit 8f123fe
Author: Daria Ivanciucova <[email protected]>
Date:   Fri Feb 2 03:20:48 2024 +0000

    TSpline rename hashed nodes  (#14921)

    * renamed 2 nodes

    TSplineSurface.BySphereCenterPointRadius
    TSplineSurface.ByCylinderPointsRadiu

    * TSplineSurface.ByPlaneOriginNormal

    * TSplineSurface.ByQuadballCenterRadius

    * TSplineSurface.BySphereFourPoints

    * TSplineSurface.ByPlaneOriginNormalXAxis

    * TSplineSurface.ByPlaneOriginXAxisYAxis

    * TSplineVertex.FunctionalValence

    * TSplineSurface.ByConeCoordinateSystemHeightRadii

    * TSplineSurface.ByPlaneBestFitThroughPoints

    * TSplineSurface.ByPlaneLineAndPoint

    * TSplineSurface.ByPlaneThreePoints

    * TSplineSurface.ByTorusCenterRadii

    * TSplineSurface.ByTorusCoordinateSystemRadii

    * TSplineTopology.TPointVertices

    * TSplineSurface.AddReflections

    * TSplineSurface.DeleteVertices

    * TSplineSurface.CompressIndices

    * TSplineSurface.RemoveReflections

    * TSplineSurface.ByNurbsSurfaceUniform

    * TSplineSurface.UnweldVertices

    * TSplineSurface.BridgeFacesToEdges

    * TSplineReflection.SegmentsCount

    * TSplineSurface.ByNurbsSurfaceCurvature

    * TSplineSurface.BridgeEdgesToFaces

    * TSplineInitialSymmetry.IsRadial; TSplineInitialSymmetry.RadialSymmetryFaces

    * TSplineReflection.SegmentsAngle

    * .TSplineSurface.BridgeFacesToFaces

    * TSplineSurface.BridgeEdgesToEdges

    * TSplineInitialSymmetry.ByRadial

    * SplineSurface.ByCombinedTSplineSurfaces

    * TSplineSurface.UncreaseVertices

    * BuildFromLines

    * TSplineSurface.CreaseVertice

    * TSplineTopology.RegularVertices

    * TSplineTopology.VertexByIndex

    * TSplineSurface.DeserializeFromTSM

    * TSplineSurface.SerializeAsTSM

    * TSplineTopology.BorderVertices

    * TSplineSurface.EnableSmoothMode

    * TSplineTopology.NonManifoldVertices

    * TSplineTopology.VerticesCoun

    * TSplineSurface.DuplicateFaces

    * TSplineSurface.ExtrudeEdgesAlongCurve

    * TSplineTopology.StarPointVertices

    * TSplineTopology.NonManifoldEdges

    * TSplineSurface.WeldCoincidentVertices

    * TSplineSurface.SubdivideFaces

    * TSplineSurface.ExtrudeFacesAlongCurve

    * TSplineSurface.ByConeCoordinateSystemHeightRadius

    * TSplineSurface.ByCylinderRadiusHeight

commit 2bb51dc
Author: Daria Ivanciucova <[email protected]>
Date:   Thu Feb 1 17:43:53 2024 +0000

    TSpline Nodes Documentation - Third set (#14598)

    * FillHole, IsClosed, IsInBoxMode

    * BorderFaces, BorderEdges, BorderVertices, DuplicateFaces

    * EnableSmoothMode, ExportToTSS, ExportToTSM

    * ImportFromTSM (2)

    * ImportFromTSS(2)

    * ExtrudeFaces, Interpolate, SubdivideFaces, FlattenVertices (2)

    * IsStandard, Standardize, Thicken (2)

    * Decomposed (3), EdgeByIndex, EdgeCount, InnerEdges, InnerFaces, InnerVertices, NGonFaces, NonManifoldEdges, NonManifoldVertices

    * RegularFaces, RegularVertices, StarPoints, TPoints

    * Repair, Serialize, Deserialize

    * ExtrudeEdges, ExtrudeEdgesAlongCurve, ExtrudeFacesAlongCurve

    * FlipNormals, IsExtractable

    * MoveVertices, FaceByIndex, VertexByIndex

    * Is Watertight, FacesCount, VerticesCount, SlideEdges, MergeEdges, WeldCoincidentVertices, WeldVertices(2)

    * ToBrep, ToMesh

    * Update WSNXB54TDQEZRTUNRFCTUDK74DHVPQPXBXEYJUZJPW6T2F7XERDQ.md

    * CreateMatch (2), PullVertices

    * MakeUniform

    * updated to better method for highlighting edges

    * spell check

    * Update 6ICXLN4V6DNK5KMYTY5LPCJBE27IRW5VOBKCCVFQGO3HST752ZNQ.md

    * added TSM, TSS files, typos

    * Helena's comments

    ---------

    Co-authored-by: Deyan Nenov <[email protected]>
    Co-authored-by: Aaron (Qilong) <[email protected]>

commit 9136633
Author: Daria Ivanciucova <[email protected]>
Date:   Thu Feb 1 16:59:41 2024 +0000

    TSpline Nodes Documentation - Second set (#14550)

    * TSplineInitialSymmetry nodes

    * TSplineReflection nodes documentation

    * two more Reflections nodes

    * 7 more nodes

    - Bevel Edges
    - Crease Edges
    - Crease Vertices
    - Uncrease Edges
    - Uncrease Vertices
    - Unweld Edges
    - Unweld Vertices

    * ByNurbs and ByCombinedSurfaces

    - ByNurbsSurfaceCurvature
    -ByNurbsSurfaceUniform
    - ByCombinedTSplineSurfaces

    * 4 Bridge nodes

    * Compress Indexes, BySweep, ByRevolve, BuildFromLines

    * DeleteEdges, DeleteFaces, DeleteVertices

    * Update Autodesk.DesignScript.Geometry.TSpline.TSplineSurface.BridgeEdgesToEdges.md

    * Update Autodesk.DesignScript.Geometry.TSpline.TSplineSurface.BridgeEdgesToFaces.md

    * Update Autodesk.DesignScript.Geometry.TSpline.TSplineSurface.BridgeFacesToEdges.md

    * Update Autodesk.DesignScript.Geometry.TSpline.TSplineSurface.BridgeFacesToFaces.md

    * Update Autodesk.DesignScript.Geometry.TSpline.TSplineSurface.BuildFromLines.md

    * added missing TSplineSurface.Reflections node

    * Helena's comments

    ---------

    Co-authored-by: Deyan Nenov <[email protected]>
    Co-authored-by: Aaron (Qilong) <[email protected]>

commit 99cf281
Author: Deyan Nenov <[email protected]>
Date:   Thu Feb 1 16:58:27 2024 +0000

    binding issues resolved (#14682)

    - fixed null exception when trying to assign click handler on a context menu button when the parent button
    - also fixed another incorrect binding issue

commit d2e57b5
Author: Daria Ivanciucova <[email protected]>
Date:   Thu Feb 1 15:58:29 2024 +0000

    TSpline Nodes Documentation - First set (#14549)

    * TSEdge and TSFace nodes documentation

    * TSpline Vertex nodes documentation

    * TSpline.UVNFrame nodes documentation

    * Update Autodesk.DesignScript.Geometry.TSpline.TSplineVertex.IsStarPoint.md

    * TSpline Box docs

    * TSplineSurface.ByPlane nodes docs

    * TSplineSurface.ByCone docs

    * TSplineSurface - Cylinder docs

    * TSplineSurface.ByQuadball nodes

    * replaced with renamed files for ByBox nodes

    * BySphere nodes

    * TSplineSurface.ByTorus nodes

    * BuildPipes and ByExtrude

    * Update Autodesk.DesignScript.Geometry.TSpline.TSplineVertex.Index.md

    * corrected the description for Torus nodes

    * Helena's comments

    ---------

    Co-authored-by: Deyan Nenov <[email protected]>
    Co-authored-by: Aaron (Qilong) <[email protected]>

* Revert "Squashed commit of the following:"

This reverts commit c182c1b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reddyashish reddyashish reddyashish approved these changes

@pinzart90 pinzart90 Awaiting requested review from pinzart90

@mjkkirschner mjkkirschner Awaiting requested review from mjkkirschner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
4 participants
@QilongTang @reddyashish @mjkkirschner @pinzart90