Do not implement props in ViewExtensionBase

[mmisol]

Purpose

This changes the definition of ViewExtensionBase to use abstract rather
than virtual for the definition of required properties like Name and
UniqueId. The use of virtual was creating a backing field which was not
used in ViewExtensionBase and could not be assigned in the inheriting
class, making it pretty much useless.

Also the Dispose method is declared as abstract, as it was already
required to implement and having a default empty implementation could
lead developers to oversight the need to dispose resources in their
extension.

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

[mmisol]

While I was making this PR @QilongTang told me the release of 2.10 is already done. Releasing this in 2.11 would make this a technically a breaking change with respect to 2.10. However, that breaking change will actually reveal issues in the extension developed, so it could be argued whether this is actually beneficial.

[aparajit-pratap]

While this looks like a good move in the right direction, it could be a breaking change, right? Ok, just saw your comment above.

[aparajit-pratap]

I actually am confused about the purpose of ViewExtensionBase as opposed to the interface, IViewExtension. Not all view extensions derive from ViewExtensionBase.

[mmisol]

I guess the discussion for the need of ViewExtensionBase came from the discussions here and here. As a summary, it is used as a way to extend the view extension API without adding a new member in the IViewExtension interface. Extension developers can implement these members by inheriting from the ViewExtensionBase class. Dynamo dynamically checks whether the extension actually implements the base class and, if that case, it calls them. For now this is used for the Closed function that was recently added.

[aparajit-pratap]

I guess the discussion for the need of ViewExtensionBase came from the discussions here and here. As a summary, it is used as a way to extend the view extension API without adding a new member in the IViewExtension interface. Extension developers can implement these members by inheriting from the ViewExtensionBase class. Dynamo dynamically checks whether the extension actually implements the base class and, if that case, it calls them. For now this is used for the Closed function that was recently added.

Ah, now I remember. Thanks for the context. In that case, it seems less risky to break this at this stage.

[mjkkirschner]

Is this a big enough issue that we should cherry pick to 2.10 and rtm that build?

…

On Dec 9, 2020, at 10:35 AM, Martin Misol Monzo ***@***.***> wrote: I guess the discussion for the need of ViewExtensionBase came from the discussions here and here. As a summary, it is used as a way to extend the view extension API without adding a new member in the IViewExtension interface. Extension developers can implement these members by inheriting from the ViewExtensionBase class. Dynamo dynamically checks whether the extension actually implements the base class and, if that case, it calls them. For now this is used for the Closed function that was recently added. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.