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 broken behavior of IsVisibleInDynamoLibrary attribute with interfaces and Enums #9242

Merged
merged 7 commits into from
Nov 14, 2018

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Nov 13, 2018

Purpose

https://jira.autodesk.com/browse/QNTM-4094

Bug description: When a class implements an interface method, although it is marked hidden, the interface method still appears as a node.

This is a regression in 2.0 and can be traced back to language changes to fix polymorphism. In this change polymorphism support was added to interfaces where the interface method was allowed to accept a derived type and the node would at runtime resolve to the concrete implementation of the method matching the derived type.

This fix is to hide the interface methods altogether to address this issue as well as to remove any confusion of having extraneous interface nodes.

When attribute IsVisibleInDynamoLibrary(false) is defined on a class the following should be the expectation:

  • The current class is hidden from both library and search
  • Interface nodes should always be hidden (even if they are implemented by derived classes) as they do not have any implementation and their derived class implementations would be exposed as nodes if needed

The second issue is with the IsVisibleInDynamoLibrary(false) attribute not taking effect on an enum, which is a regression from 1.3.X. This can be traced back to the change made here . This too has been fixed, which includes the change to not have to individually set the attributes on each enum field. The attributes, IsVisibleInDynamoLibrary and Obsolete can now be applied globally on the enum, which now takes effect on all of the fields as well.

Declarations

Check these if you believe they are true

  • The code base 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

@aparajit-pratap aparajit-pratap changed the title fix broken behavior of IsVisibleInDynamoLibrary attribute with interf… Fix broken behavior of IsVisibleInDynamoLibrary attribute with interfaces Nov 13, 2018

[IsVisibleInDynamoLibrary(false)]
[Serializable]
public class TraceableId : ISerializable
Copy link
Contributor

Choose a reason for hiding this comment

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

@aparajit-pratap Can you explain how this test works, I can't see how adding a class with the tag verify stuff from my knowledge.

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 I need to think how I can test for interfaces not appearing in search and library.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aparajit-pratap What do you think of either screen comparison or search results comparison?

@@ -354,6 +354,10 @@ private ClassDeclNode ParseSystemType(Type type, string alias)
CLRModuleType.GetInstance(interf, Module, string.Empty);
}
}
if(classnode.IsInterface)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you able to track down why this got regressed? I do not see this check existed in previous version of code

@aparajit-pratap aparajit-pratap changed the title Fix broken behavior of IsVisibleInDynamoLibrary attribute with interfaces Fix broken behavior of IsVisibleInDynamoLibrary attribute with interfaces and Enums Nov 14, 2018
@QilongTang
Copy link
Contributor

LGTM

@QilongTang QilongTang added the LGTM Looks good to me label Nov 14, 2018
@aparajit-pratap aparajit-pratap merged commit 5d05bc9 into DynamoDS:master Nov 14, 2018
@aparajit-pratap aparajit-pratap deleted the isVisible branch November 14, 2018 18:02
@QilongTang
Copy link
Contributor

@aparajit-pratap Considering this bug should be there in Dynamo 2.0.2 as well. Do you mind cherry-picking to RC2.0.2 as well?

aparajit-pratap added a commit to aparajit-pratap/Dynamo that referenced this pull request Nov 14, 2018
…aces and Enums (DynamoDS#9242)

* fix broken behavior of IsVisibleInDynamoLibrary attribute with interfaces

* revert extraneous changes

* fixes

* added test

* revert file

* fix regression in IsVisibleInDynamoLibrary attribute applied to enum

* add test for enum attribute fix
aparajit-pratap added a commit that referenced this pull request Nov 14, 2018
…aces and Enums (#9242) (#9247)

* fix broken behavior of IsVisibleInDynamoLibrary attribute with interfaces

* revert extraneous changes

* fixes

* added test

* revert file

* fix regression in IsVisibleInDynamoLibrary attribute applied to enum

* add test for enum attribute fix
{
HiddenInLibrary = true;
ObsoleteMessage = (attr as IsObsoleteAttribute).Message;
ObsoleteMessage = (attr as ObsoleteAttribute).Message;
Copy link
Member

Choose a reason for hiding this comment

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

@aparajit-pratap what is the difference between these attribute types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IsObsolete is written by us and Obsolete is a standard attribute.

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