GeometryFactoryPath lookup should be more consistent with other preloader utilities. #9493
Conversation
should harden / define edge cases and add tests next
define edge cases
mjkkirschner
changed the title
|
|
||
|
|
||
| //when passed a null version this method should return empty string - as the geometry path was not located | ||
| Assert.AreEqual(string.Empty, |
There was a problem hiding this comment.
please see these cases - looking for feedback if these make sense.
| public static string GetGeometryFactoryPathTolerant(string rootFolder, Version tolerantVersion){ | ||
| if(tolerantVersion != null && rootFolder != null) | ||
| { | ||
| return Path.Combine(Utilities.GetLibGPreloaderLocation(tolerantVersion, rootFolder), Utilities.GeometryFactoryAssembly); |
There was a problem hiding this comment.
The code itself looks fine but I was expecting us to merge some code between GetGeometryFactoryPath2 and GetGeometryFactoryPathTolerant because in the end either DynamoCore or our client would only be available to call one of them. But for now the code path between these two are quite different. From the API name I would assume this one is only doing the fall back logic as addition? Do you think if we also still need those folder check logic in this new API?
There was a problem hiding this comment.
-
I find these two methods both potentially useful - they do different things, and I think it's always useful to have an explicit method to avoid unknown behavior which is why I did not obsolete the other method.
-
Sandbox AFAIK does not call these methods at all.
-
The end result to the client is like you said that it appears it does a fallback - it DOES have different edge case behavior though - it returns a empty string instead of exceptions when it encounters bad input - that's kind of what I was asking about in the test cases? I'm okay with the more slow failure because this is a
tolerantversion of the method, but open to other opinions for sure.
There was a problem hiding this comment.
Sure, I don't mean to obsolete the other one. I thought we want to include the folder check logic in the new API as well because of some legacy bug, if you think it is OK and good enough for the clients then this version should be fine to use. Do you plan to make any change in client code as an example usage of this API later when nuget is up?
There was a problem hiding this comment.
I actually don't see any reason to keep GetGeometryFactoryPath2 after the addition of this new API and I think we should add the folder check exceptions to the new API. I don't see reasons to remove them from here. GetGeometryFactoryPath2 can give unexpected results (in combination with other API's) like we've seen in the Revit case so I think it should be obsoleted.
|
Thanks for the feedback @QilongTang and @aparajit-pratap - I will take another crack at reconciling these apis tomorrow. |
add exception throwing for null or bad inputs define expected outputs in tests
|
@QilongTang @aparajit-pratap @scottmitchell - went through this again and after it seems to avoid adding any new APIs we can just make the existing Clients should not need to update their usage but they will get more robust behavior. The tests have been updated to verify the edge cases with bad inputs and the comments have been updated for all methods that wrap this method internally. |
|
@mjkkirschner excellent tests! Thanks, LGTM! |
| var rootFolder = Path.Combine(Path.GetTempPath(), "LibGTest"); | ||
| // both versions of libG exist | ||
| var libG22500path = System.IO.Directory.CreateDirectory(Path.Combine(rootFolder, "LibG_225_0_0")); | ||
| var libGTestPath = new DirectoryInfo(Path.Combine(rootFolder, "LibG_224_24_24")); |
There was a problem hiding this comment.
You end up not using this, right? Also, the "both versions of libG exist" comment in this test threw me off as I was reading through it.
There was a problem hiding this comment.
thanks @scottmitchell will fix this.
| } | ||
|
|
||
| //lookup libG with a fallback to older versions which share the major version number. | ||
| var libGFolder = Utilities.GetLibGPreloaderLocation(version, rootFolder); |
There was a problem hiding this comment.
With the current implementation of GetGeometryFactoryPath2 I feel that we can remove the preloadASM() function in D4R since the default call of GetGeometryFactoryPath will make the fall back as well, what do you think?
There was a problem hiding this comment.
yes, I think their implementation can go back to how it was previously.
|
The current implementation makes sense to me in a way that it is very clear to a client which API to use. LGTM |
|
LGTM! |
|
@QilongTang @AndyDu1985 @ZiyunShang - verified this fixes the issue loading ASM on on latest revit build - I confirmed this by moving back before this PR: |
Purpose
Provide a more tolerant GeometryFactoryPath lookup method that falls back to other libG/ASM version combinations that are <= to the major version requested.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@QilongTang @scottmitchell
FYIs
@aparajit-pratap