Conversation
QilongTang
added
PTAL
QilongTang
changed the title
|
@mjkkirschner @aparajit-pratap @angelowang Thank you guys for the review, I addressed all your comments and this is ready for another look |
| preloaderVersions.Reverse(); | ||
| // Pick the closest preloader version below or use the default value when no libG folder found | ||
| var preloaderVersion = preloaderVersions.FirstOrDefault() == null ? version : preloaderVersions.First(); | ||
| return Path.Combine(rootFolder, string.Format("libg_{0}_{1}_{2}", preloaderVersion.Major, preloaderVersion.Minor, preloaderVersion.Build)); |
There was a problem hiding this comment.
If there's no matching libg folder found for the target version passed in, then setting preloaderVersion to version and constructing a libg folder path from it will return a non-existent location.
There was a problem hiding this comment.
@aparajit-pratap Preload() will fail in this case, do you prefer just fall back to whatever libG folder available even the version is larger?
There was a problem hiding this comment.
@aparajit-pratap
After deeper look of everything together, I think we will end up having invalid path only in two cases:
- User only has a libG folder with higher major version
- User deleted all the local libG folders
Although unlikely, when these two cases happen, we will throw exception at
which will end up in the analytics data. I have made the exception message more explicit, but at this point DynamoViewModel has not been initialized so there is no crash dialog.I think it is fine this way because the two cases I mentioned is very less likely to happen unless user touches the Dynamo Installation folder themselves.
|
@QilongTang I just have one more comment; the last one above that I think needs to be addressed. After that this is good to go. |
|
@QilongTang @aparajit-pratap is this good to go? |
|
@mjkkirschner Not yet, @aparajit-pratap wanted to add some better error message when libG preloader not found so I am looking into it |
|
Thanks @QilongTang, LGTM! |
* Add a new static function for preloader lookup * Find the cloest version of preloader from ASM version * Add Unit Test * Address comments * Fix null case and regression * Update Unit test and address comments * Update exception message
4 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
This is an improvement when working with a beta release of ASM release that we make our libG preloader lookup mechanism more tolerant than just search for the exact match.
The influence of this API is that for in-process Dynamo integration, client need to add this API call to their ASM preloading logic:
Declarations
Check these if you believe they are true
*.resxfilesRunning self-CI
Reviewers
@angelowang @mjkkirschner
FYIs
@aparajit-pratap @alfarok