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

Fix API breaking changes #14916

Merged
merged 4 commits into from
Feb 2, 2024
Merged

Fix API breaking changes #14916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d1af39a
Fix API breaking changes
QilongTang Jan 31, 2024
9e53a2b
Merge branch 'master' into APIBreakingChange
QilongTang Feb 1, 2024
d88d273
Update OpenFileCommand
QilongTang Feb 1, 2024
738d40e
update
QilongTang Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/DynamoCore/Models/RecordableCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,13 +457,25 @@ public class OpenFileCommand : RecordableCommand
{
#region Public Class Methods

/// <summary>
/// Constructor
/// </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.

{
FilePath = filePath;
ForceManualExecutionMode = forceManualExecutionMode;
IsTemplate = false;
}

/// <summary>
/// Constructor
/// </summary>
/// <param name="filePath">The path to the file.</param>
/// <param name="forceManualExecutionMode">Should the file be opened in manual execution mode?</param>
/// <param name="isTemplate">Is Dynamo opening a template file?</param>
public OpenFileCommand(string filePath, bool forceManualExecutionMode = false, bool isTemplate = false)
public OpenFileCommand(string filePath, bool forceManualExecutionMode, bool isTemplate)
{
FilePath = filePath;
ForceManualExecutionMode = forceManualExecutionMode;
Expand Down
2 changes: 1 addition & 1 deletion src/DynamoCoreWpf/TestInfrastructure/CustomNodeMutator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand Down
2 changes: 1 addition & 1 deletion test/DynamoCoreTests/Engine/LiveRunnerServicesTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.IO;
using System.IO;
using Dynamo.Models;
using NUnit.Framework;

Expand Down
2 changes: 1 addition & 1 deletion test/DynamoCoreTests/Models/ForceRunCancelCommandTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.IO;
using System.IO;
using System.Xml;
using Dynamo.Models;
using Dynamo.Utilities;
Expand Down
Loading