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

Fix Publish package remove item bug #12941

Merged
merged 2 commits into from
May 31, 2022
Merged

Conversation

zeusongit
Copy link
Contributor

Purpose

Fix a bug in Dynamo Publish Package window, where user was unable to remove an item after publishing it it the next version. The problem was that the PackageContent list was being refreshed, bringing all the concatenated files back, including the deleted ones, therefore now we are updating each separate file list as well, so that a PackageContent refresh won't affect the delete action.

Revit_Pl22aPmiz0

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.
  • This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@QilongTang
Copy link
Contributor

@zeusongit So this is not a new regression?

@zeusongit
Copy link
Contributor Author

@zeusongit So this is not a new regression?

No, apparently this used to happen before as well, I guess somehow was never discovered.

Copy link
Contributor

@QilongTang QilongTang 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 but can we add a unit test since we never knew this regressed?

@QilongTang QilongTang added this to the 2.15.0 milestone May 27, 2022
string fileName = packageItemRootViewModel.FileInfo == null ? packageItemRootViewModel.Name : packageItemRootViewModel.FileInfo.FullName;
string fileType = packageItemRootViewModel.DependencyType.ToString();

if (fileName.ToLower().EndsWith(".dll") || fileType.Equals(DependencyType.Assembly))
Copy link
Contributor

Choose a reason for hiding this comment

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

If the filetype is DependencyType.Assembly, I suppose it should be a dll. Do we need both the checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was the case before, but I noticed that we do not get filename with .dll extension, therefore added another check that will satisfy this condition, putting an && will not pass if the filename is without extension.

Copy link
Member

@mjkkirschner mjkkirschner May 31, 2022

Choose a reason for hiding this comment

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

what about packages that ship .exes? I'm not sure if that was allowed before, but if it was this might be a breaking change?
ahh, did not see the ||

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the current flow, they will end up in additional files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to add .exe extension to assemblies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do exes have Dependency Type as Assembly?

Copy link
Member

@mjkkirschner mjkkirschner May 31, 2022

Choose a reason for hiding this comment

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

I think not - according to:

return Directory.EnumerateFiles(RootDirectory, "*.dll", SearchOption.AllDirectories);

we only scan files ending in *.dll and assign those types as assembly. I guess like you said above .exes would just be another file in extra.

I don't think theres any reason to change that now, just wanted to avoid making any inadvertent changes near to release.

Copy link
Contributor Author

@zeusongit zeusongit May 31, 2022

Choose a reason for hiding this comment

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

I published a test package with exes, it ends up in additional files, so we are good.
thanks for the review @reddyashish @mjkkirschner

Copy link
Contributor

@reddyashish reddyashish 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 with one comment.

@zeusongit zeusongit merged commit 1d5b1db into DynamoDS:master May 31, 2022
zeusongit added a commit to zeusongit/Dynamo that referenced this pull request Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants