[Cherry-Pick] DYN-9246 Remove Dynamo API marked Obsolete in 1.x (#16654) #16687
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-9246
There was a problem hiding this comment.
Pull Request Overview
This PR removes obsolete APIs that were marked for deprecation in Dynamo 1.x, specifically targeting 18 user-facing node functions and various internal APIs marked with [Obsolete] attributes. The changes represent a breaking API cleanup as part of technical debt reduction.
Key Changes:
- Removal of 18 deprecated user-facing nodes across CoreNodes, Analysis, and DSOffice libraries
- Elimination of obsolete internal APIs including
LibraryVersionenum,IsBackgroundPreviewActiveproperty, and deprecated runner classes - Updated test code to use replacement APIs (
BackgroundPreviews.Count > 0instead ofIsBackgroundPreviewActive)
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Watch3DViewModelTests.cs | Updated tests to use BackgroundPreviews.Count > 0 pattern instead of obsolete IsBackgroundPreviewActive property |
| TestSessionConfiguration.cs | Removed obsolete RequestedLibraryVersion property and unused LibraryVersion enum variable |
| CoreUITests.cs | Replaced obsolete API calls with BackgroundPreviews.Count > 0 and removed obsolete property setters from preference tests |
| Preloader.cs | Removed obsolete LibraryVersion enum and Version property |
| HydrogenRunner.cs | Deleted entire obsolete runner class file |
| DeuteriumRunner.cs | Deleted entire obsolete runner class file |
| PackageManagerClient.cs | Removed obsolete package binary/Python detection constants |
| DefaultWatch3DViewModel.cs | Updated initialization to use BackgroundPreviews.Count instead of obsolete property |
| PackageManagerClientViewModel.cs | Simplified package content detection by removing checks for obsolete constants |
| ModelBase.cs | Removed obsolete warnings from Serialize and Deserialize methods |
| PreferenceSettings.cs | Removed obsolete IsBackgroundPreviewActive property implementation |
| IPreferences.cs | Removed obsolete IsBackgroundPreviewActive property declaration from interface |
| public void PreferenceSetting_BackgroundPreview_1_0API() | ||
| { | ||
| bool expectedValue = !ViewModel.Model.PreferenceSettings.IsBackgroundPreviewActive; | ||
| bool expectedValue = !(ViewModel.Model.PreferenceSettings.BackgroundPreviews.Count > 0); |
There was a problem hiding this comment.
[nitpick] The expression BackgroundPreviews.Count > 0 is repeated multiple times throughout the test. Consider extracting it into a helper method or property (e.g., IsBackgroundPreviewActive()) to improve maintainability and make the intent clearer.
| public void Watch3DViewModel_Active_InSyncWithPreferencesUsing1_0API() | ||
| { | ||
| Assert.AreEqual(ViewModel.BackgroundPreviewViewModel.Active, ViewModel.Model.PreferenceSettings.IsBackgroundPreviewActive); | ||
| Assert.AreEqual(ViewModel.BackgroundPreviewViewModel.Active, ViewModel.Model.PreferenceSettings.BackgroundPreviews.Count > 0); |
There was a problem hiding this comment.
[nitpick] The expression BackgroundPreviews.Count > 0 is used repeatedly as a replacement for the removed API. Consider creating a helper method to encapsulate this logic and improve code readability.
| var contains_python = | ||
| !are_contents_empty && x.contents.Contains(PackageManagerClient.PackageContainsPythonScriptsConstant); | ||
| var contains_binaries = x.contains_binaries || !are_contents_empty; | ||
| var contains_python = !are_contents_empty; |
There was a problem hiding this comment.
The logic for contains_python now simply checks !are_contents_empty, which means any non-empty content is treated as containing Python. This appears overly broad and may not accurately reflect whether a package actually contains Python scripts. Consider whether this logic should be refined or if additional checks are needed.
| var contains_python = !are_contents_empty; | |
| // Refined logic: check if contents mention any .py files (case-insensitive) | |
| var contains_python = !are_contents_empty && x.contents.IndexOf(".py", StringComparison.OrdinalIgnoreCase) >= 0; |
|
Cherry picked from RC4.0.0_master I mistakenly created a PR for these changes in the release branch first |
Is that why the cherry-pick automation you were trying did not work? |
This was the manual cherry-pick, which did work. (Although the checks seems to be failing.) The one that did not work was the cherry-pick from master to RC4.0.0_master. |
jasonstratton
changed the title
|
SynamoSelfServe runs with 5 failed tests. 3 have been fixed already with other PR. Not sure why these 2 are failing: |
…ked-Obsolete-in-1.x-(DynamoDS#16654)
It tests the API in 1.0, which was removed.. There is a test that does test the newer API, PreferenceSetting(), in the same file
Watch3DViewModel_Active_InSyncWithPreferencesUsing1_0API() tests the 1.0 API, which was removed Watch3DViewModel_Active_InSyncWithPreferences() in the same file tests the newer API
3 participants
Purpose
Remove API marked obsolete in Dynamo 1.x from Dynamo 4.0 - DYN-9246 - updated 18 items in total
Declarations
Check these if you believe they are true
Release Notes
Removes 18 user-facing node functions that were marked with [NodeObsolete] in Dynamo 1.3.4:
CoreNodes (6 items):
Analysis (11 items):
DSOffice (1 item):