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

Revert "3dview wrapper 2018" #2283

Merged
merged 1 commit into from
Dec 10, 2018
Merged

Conversation

ZiyunShang
Copy link
Collaborator

@ZiyunShang ZiyunShang commented Dec 7, 2018

Reverts #1999

An API breaking change is made, the main 3d view classes have their base class changed. Any code that relied on the base class now breaks.
DynamoRaas functionality also now is broken because those nodes expected view3d types and now all 3d views have a base class of Abstract3dView.

FYI: @ksobon

@ksobon
Copy link
Contributor

ksobon commented Dec 7, 2018

@ZiyunShang Please list what functionality was broken by this PR, and we should:

  • fix the broken code, and re-implement my solution, because as @andydandy74 has mentioned it is needed by the community.
  • create tests to prevent this from happening in the future
  • do a better job reviewing and approving PR's

@mjkkirschner
Copy link
Member

mjkkirschner commented Dec 7, 2018

Hi @ksobon - please see:
https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net
https://semver.org/

your PR refactored the view classes with a new base class.

This means any code that used to expect view types to inherit from View3d will no longer work. It's a matter of semantic versioning that we should avoid removing or modifying public method or type names between major versions of dynamoRevit releases.

I'm sorry we missed this in the PR.

Maybe the fix can still be made without the new base classes - or by adding an interface as opposed to replacing the base class? - OR including your new abstract class as the real base class in the chain. I think that last suggestion might still break.

@mjkkirschner
Copy link
Member

Also automating the checking of backwards compatibility is something that can be done with
https://blogs.msdn.microsoft.com/bclteam/2005/03/21/using-reflection-to-list-version-to-version-assembly-changes-libcheck-one-of-the-best-tools-ever-kit-george/

or ApiChange tools - it's possible but not trivial amount of work and not full proof I think.

@AndyDu1985
Copy link
Collaborator

This fix is OK, but it is an API breaking change. We can file a new PR for this fix and merge it when new major, such as version 3.x, is ready.

@AndyDu1985 AndyDu1985 merged commit 90fbb82 into Revit2018 Dec 10, 2018
@ZiyunShang ZiyunShang deleted the revert-1999-3dview-wrapper-2018 branch December 10, 2018 08:05
@ZiyunShang
Copy link
Collaborator Author

We would merge the 3dview-wrapper commits after version 3.x. Track it with link: #2290 .

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