[DYN-2870] Increase the code coverage: DynamoCore/Graph Folder

[Astul-Betizagasti]

Purpose

Remove unused code in the Workspaces sub folder

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.

Reviewers

Aaron Tang (@QilongTang )
Michael Kirschner (@mjkkirschner )

FYIs

Aldredo Pozo (@alfredo-pozo )

[QilongTang]

@DynamoDS/dynamo any comments? I think this is safe to remove since we are no longer serializing the whole workspace using XML. We still preserve the node serialization logic using XML for undo/redo which is un touched.

[mjkkirschner]

unfortunately ... removing a protected member from a public class probably constitutes an API change? Maybe not a break... - If some client inherited from this class, and used that method, it will now call the base class right and behavior will be significantly different?

[mjkkirschner]

always good to read this again:
https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/breaking-change-rules.md

I don't see protected members called out specifically...

[QilongTang]

How about this

❌ DISALLOWED: Reducing the visibility of a member

This includes reducing the visibility of a protected member when there are accessible (public or protected) constructors and the type is not sealed. If this is not the case, reducing the visibility of a protected member is allowed.

Increasing the visibility of a member is allowed.

From https://docs.microsoft.com/en-us/dotnet/core/compatibility/. I guess removing is part of reducing visibility of protected member? I really would like to remove it :(

[mjkkirschner]

well... does removing it count as lowering its visibility 😉 ?
I think the API will still be callable, an exception will not be thrown at compile or runtime - but behavior will potentially change. For example, I could imagine Dynamo Player uses this API.

[QilongTang]

@mjkkirschner So obsoletion then?

[mjkkirschner]

that would be the safe bet.

[QilongTang]

@Astul-Betizagasti Can you obsolete these methods then, their should be plenty example in the code base about obsolete messaging. Sorry about the back and forth.

[Astul-Betizagasti]

Ok, no problem I'll change it. Should I add a message on the Obsolete attribute or there is no need?

Edit: my browser had not refreshed and I didn't see you previous message

[QilongTang]

@Astul-Betizagasti Yes

[Obsolete("Method will be deprecated in Dynamo 3.0.")]

I dont think there is a replacement on workspace model.. We can append use ToJson() or Save() but they are not the same in all ways.

[aparajit-pratap]

Why can't we remove this right away?

[QilongTang]

@Astul-Betizagasti For private functions we should be able to remove any time. unlike protected and public ones