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

PlanarSurface is missing InternalElements2D #2380

Closed
pawelbaran opened this issue Mar 9, 2021 · 2 comments
Closed

PlanarSurface is missing InternalElements2D #2380

pawelbaran opened this issue Mar 9, 2021 · 2 comments
Assignees
Labels
type:bug Error or unexpected behaviour

Comments

@pawelbaran
Copy link
Member

Description:

As in the title, PlanarSurface is missing InternalElements2D method, which means that calling IElement2D.IInternalElements2D on it returns an empty list and the failure is silent (or will be after #2378 gets merged). Another brick to the wall we are building around RunExtensionMethod @IsakNaslundBh @alelom 😃

I am assigning a few individuals to make sure we are all in the loop - it seems that the pattern may need a wider discussion and careful tweaks.

@IsakNaslundBh
Copy link
Contributor

PlanarSurface is not an IElement2D though. From memory, the issue here is the exact this reason why, so this is not really a bug but a missing feature. Makes me less worried about this, and less urgent IMO, even though we need to resolve it.

It needs to be handled by a slight restructuring of the Planar surface though, as the internalBoundaries right now are ICurves, and those can ofc not be IElement2D.

@pawelbaran
Copy link
Member Author

Fair point @IsakNaslundBh, my deduction mistake. However, this case still highlights the problem of the error being hidden, allowing for lack of method implementation to come unnoticed (if PlanarSurface actually implemented IElement2D without dedicated InternalElements2D, it could not be seen until some other method based on it would return wrong results).

Maybe I am overreacting, but in this case (InternalElements2D) we base the correctness of the code on the developer's knowledge of the framework, which might be a bit dangerous, particularly in case of new team members. Similar would actually apply to Transform proposed in #2372, where the contributor needs to know that the type-specific Transform method needs to be added if it goes beyond the simple location update of an IElement. I am not sure how to deal with that without forcing every type to have a specific implementation.

I will close this issue, happy to open a new one, focused on the problem described above if anyone else thinks we need a wider discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
Development

No branches or pull requests

7 participants