Skip to content

Conversation

@rdeits
Copy link
Contributor

@rdeits rdeits commented May 7, 2020

This PR adds compatibility with Rotations.jl v1.x. The changes turned out to be trivial: there's just one place where this package re-exports some Rotations.jl types, so we need to handle the new types there.

@c42f
Copy link
Member

c42f commented May 7, 2020

Awesome, thanks!

A thought - maybe we should delete these exports rather than update them? People can always do using Rotations if they want them in their namespace.

@andyferris
Copy link
Contributor

I agree... these exports seem quite likely to be a hangover from the very beginnings of this package where we may have wanted to provide derivatives for these somehow.

How "breaking" (in a semver sense) is it to remove the dependency and reexport of Rotations.jl? In either case I think it's worthwhile.

@c42f c42f mentioned this pull request May 7, 2020
3 tasks
@c42f
Copy link
Member

c42f commented May 7, 2020

Well, it definitely breaks people's code unless they also have using Rotations, I'm not sure it can be deprecated either? Somehow we'd need to deprecate bindings which are created in the user's module from the using builtin, but only if they arise from CoordinateTransformations and not from Rotations. Actually, may be do-able, by explicitly introducing bindings by hand inside CoordinateTransformations with @deprecate_binding. I'm unsure though.

@rdeits what do you need at your end? At this stage these re-exports leave us in a really weird state where CoordinateTransformations can't release a Rotations-1.0-compatible version without breaking its own public API!

@rdeits
Copy link
Contributor Author

rdeits commented May 12, 2020

Sorry I missed this--I'm perfectly happy for the exports to go. My actual motivation was to get MeshCat.jl compatible with Rotations.jl, and the compatibility in CoordinateTransformations was blocking that. I just made the most conservative and compatible PR I could make 🙂

I'm happy to go with #61 instead.

@c42f
Copy link
Member

c42f commented May 12, 2020

Cool, let's close this in favor of breaking the dependency entirely then.

@c42f c42f closed this May 12, 2020
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.

3 participants