Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
GeometryFactoryPath lookup should be more consistent with other preloader utilities. #9493
GeometryFactoryPath lookup should be more consistent with other preloader utilities. #9493
Changes from 2 commits
1628349
fc58008
ab62561
4caffac
437a28c
db31de1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code itself looks fine but I was expecting us to merge some code between
GetGeometryFactoryPath2
andGetGeometryFactoryPathTolerant
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
tolerant
version of the method, but open to other opinions for sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @scottmitchell will fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see these cases - looking for feedback if these make sense.