Skip to content
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

AGD-1437 Can not preload a package with an apostrophe in its the folder path. #10685

Merged
merged 3 commits into from
May 21, 2020

Conversation

saintentropy
Copy link
Contributor

@saintentropy saintentropy commented May 20, 2020

Purpose

This PR is to resolve an issue found with localized French samples for GD where the included packages would not load into Dynamo. We found while these packages will load during runtime (ie if the user adds a folder to the "Manage Node and Package Paths" dialog) the package will not load upon a restart of Dynamo. This PR adds a fix for the formatted string path which includes an apostrophe used to preload packages during start up.

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

@QilongTang @mrahmaniasl

FYIs

@mjkkirschner

@@ -23,6 +23,9 @@ public static string GetDSFullPathName(string fileName, Options options = null)
var trimChars = new[] {'\n','\t','\r',' '};
fileName = fileName.Trim(trimChars);

// Fix file paths which include an apostrophe
fileName = fileName.Replace("\\'", "'");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand it correctly this is replacing \' with ', but how would the test case reflect this fix? Maybe I missed something?

{
LibraryLoaded = false;

string libraryPath = Path.Combine(TestDirectory, "pkgs", "Pack'age", "bin", @"Package.dll");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QilongTang This test fails with out the formatting fix

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saintentropy maybe I misunderstand the issue, how would fileName.Replace("\\'", "'") help this case?

Copy link
Contributor Author

@saintentropy saintentropy May 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry now I get your question @QilongTang. The path here gets modified here

https://github.com/DynamoDS/Dynamo/blob/master/src/Engine/ProtoCore/Utils/CompilerUtils.cs#L258 in TryLoadAssemblyIntoCore

The return value of ToLiteral is modified from "Pack'age" to "Pack\\'age" when it is parsed here https://github.com/DynamoDS/Dynamo/blob/master/src/Engine/ProtoCore/Utils/CompilerUtils.cs#L242

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above is one example of the of the call to ToLiteral

@QilongTang
Copy link
Contributor

@saintentropy or @mrahmaniasl Can you also make it clear if this needs to go into Revit 2021.1 or can be included in the later point release?

@saintentropy
Copy link
Contributor Author

@QilongTang Up to you regarding release. This was originally filed by localization but is not a blocker for GD

Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I'll wait for @QilongTang review before tagging as LGTM

@QilongTang
Copy link
Contributor

LGTM

@QilongTang QilongTang added the LGTM Looks good to me label May 21, 2020
@QilongTang QilongTang merged commit 8be5ca7 into DynamoDS:master May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants